Hi all-
I am mostly doing work in Clang (and am new there), so I apologize if this isn't the proper place to mention this. I appreciate guidance in advance.
I was looking into some of the unit tests, and noticed that the ArrayRefTest.InitializerList, and thus the InitializerList constructor of ArrayRef (under normal use-case) hit undefined behavior. The test does the following:
ArrayRef<int> A = { 0, 1, 2, 3, 4 };
for (int i = 0; i < 5; ++i)
EXPECT_EQ(i, A[i]);
For those unfamiliar, ArrayRef is a T* Data/size_t Length pair-type with a std::initializer_list Ctor that simply copies the initializer_list::begin into Data.
The issue is that after the assignment, the initializer-list temporary goes out of scope (since it is a temporary), creating a dangling pointer. This doesn't seem to be an issue for the most part, however compiling the test with -O2 and -fno-merge-all-constants causes this test to fail.
I suspect that this should be fixed in 1 of the following ways. I'm willing to contribute the patch, but would like some guidance as to which the community thinks is the proper solution.
1- "Delete" r-value ctors for ArrayRef. I did a quick test just deleting r-value std;:initializer list, and discovered quite a few usages of construct-from-temporary (before the build gave up!) that would need to be fixed as well.
2- Implement the r-value ctors to allocate. This is likely going to require an additional member to capture the fact that this was allocated and thus needs to be free'd. I suspect that this violates the purpose of the ArrayRef.
3- Others?
Thanks,
Erich Keane
Hi all-
I am mostly doing work in Clang (and am new there), so I apologize if this isn't the proper place to mention this. I appreciate guidance in advance.
I was looking into some of the unit tests, and noticed that the ArrayRefTest.InitializerList, and thus the InitializerList constructor of ArrayRef (under normal use-case) hit undefined behavior. The test does the following:
ArrayRef<int> A = { 0, 1, 2, 3, 4 };
for (int i = 0; i < 5; ++i)
EXPECT_EQ(i, A[i]);
For those unfamiliar, ArrayRef is a T* Data/size_t Length pair-type with a std::initializer_list Ctor that simply copies the initializer_list::begin into Data.
The issue is that after the assignment, the initializer-list temporary goes out of scope (since it is a temporary), creating a dangling pointer. This doesn't seem to be an issue for the most part, however compiling the test with -O2 and -fno-merge-all-constants causes this test to fail.
I suspect that this should be fixed in 1 of the following ways. I'm willing to contribute the patch, but would like some guidance as to which the community thinks is the proper solution.
1- "Delete" r-value ctors for ArrayRef. I did a quick test just deleting r-value std;:initializer list, and discovered quite a few usages of construct-from-temporary (before the build gave up!) that would need to be fixed as well.
How would we do with ArrayRef as function argument, built from an R-value? Sounds like a valid use-case to me.
2- Implement the r-value ctors to allocate. This is likely going to require an additional member to capture the fact that this was allocated and thus needs to be free'd. I suspect that this violates the purpose of the ArrayRef.
Right.
Note that I believe the same issue exists with Twine and StringRef.
Sorry for the inline-comment format being weird, I haven't figured out yet how to do '>' stuff in outlook yet 
Hopefully this is clear enough.
Yeah, this general issue comes up across the board. Generally it’s “don’t do that” and we could add some clang-tidy checks to help catch these cases (perhaps even powered by an attribute on the type so it’s extensible).
which is to say: fix the test case, it’s buggy, but that’s about it
Ok, thanks for the guidance. I’ll submit a review this afternoon to fix the test case. I spent most of the morning to try to find some violations of lifetimes around the code, but didn’t see any smoking guns. For the most part, ArrayRef is either used properly, or the issue is deep enough call-wise I didn’t see it easily enough.