This is an archive of the discontinued LLVM Phabricator instance.

Allow flexible array initialization in C++.
ClosedPublic

Authored by efriedma on Apr 12 2022, 5:42 PM.

Details

Summary

Flexible array initialization is a C/C++ extension implemented in many compilers to allow initializing the flexible array tail of a struct type that contains a flexible array. In clang, this is currently restricted to C. But this construct is used in the Microsoft SDK headers, so I'd like to extend it to C++.

For now, this doesn't handle dynamic initialization; probably not hard to implement, but it's extra code, and I don't think it's necessary for the expected uses. And I'm not handling constant evaluation; it should fail gracefully.

I've added some additional code to assert that flexible array init works the way we expect it to in both C and C++.

Diff Detail

Event Timeline

efriedma created this revision.Apr 12 2022, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 5:42 PM
efriedma requested review of this revision.Apr 12 2022, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 5:42 PM
erichkeane added inline comments.Apr 13 2022, 6:04 AM
clang/lib/CodeGen/CGDecl.cpp
346

Can you write a test for this with a 'fixme'? I don't see a reason why we shouldn't support this eventually.

clang/lib/CodeGen/CodeGenModule.cpp
4618

Same comment here.

clang/test/SemaCXX/constant-expression-cxx11.cpp
2388

I would expect this to be an error, not the static-assert. The constexpr variable means 'initializable as a constant expression'.

I'm guessing the problem is ACTUALLY that we support constexpr init, but not the operator[]. I think I'd like to have us have the initialization fail here, since it isn't otherwise usable.

efriedma added inline comments.Apr 13 2022, 12:31 PM
clang/test/SemaCXX/constant-expression-cxx11.cpp
2388

I think we end up computing the initializer "correctly", but have no way to actually access the elements in constant evaluation. Seems to come out of CGExprConstant lowering okay, but I guess we don't really need that to work at the moment; we fall back to the old CGExprConstant direct lowering code.

I'll add a bailout to constant evaluation.

efriedma updated this revision to Diff 422671.Apr 13 2022, 2:57 PM

Address review comments

erichkeane accepted this revision.Apr 13 2022, 5:04 PM
This revision is now accepted and ready to land.Apr 13 2022, 5:04 PM
This revision was landed with ongoing or failed builds.Apr 14 2022, 11:57 AM
This revision was automatically updated to reflect the committed changes.

Apparently the assertions I added are triggering in the LLVM test-suite. Commented them out in 6cf0b1b3da3e8662baf030a2d741e3becaaab2b0; I'll come up with a proper fix soon.

For posterity, posting the link to the failure this caused in Linaro's TCWG's CI of kernel builds w/ clang: https://lore.kernel.org/llvm/906914372.14298.1650022522881@jenkins.jenkins/.

I'm guessing D123826 fixes this?

6cf0b1b3 was a temporary fix to stop the crash. D123826 is the full fix to make the assertion actually work correctly.

ahatanak added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
4639

This assertion fails when the following code is compiled:

typedef unsigned char uint8_t;
typedef uint8_t uint8_a16 __attribute__((aligned(16)));

void foo1() {
  static const uint8_a16 array1[] = { 1 };
}
efriedma added inline comments.May 17 2022, 12:22 PM
clang/lib/CodeGen/CodeGenModule.cpp
4639

sizeof(uint8_a16[1]) is 16, but we currently emit a one-byte global. So it seems like there's an underlying bug exposed by the assertion.

gcc thinks this is nonsense, and just prints an error.

ahatanak added inline comments.May 18 2022, 4:53 PM
clang/lib/CodeGen/CodeGenModule.cpp
4639

It seems to me that we should disallow arrays that have an element whose alignment is larger than its size, just as gcc does.

But I see arrays like that declared in a couple of clang's regression tests (e.g., Sema/attr-aligned.c). I'm not sure whether there was a reason for not rejecting it or it was just an oversight.

efriedma added inline comments.May 19 2022, 2:27 PM
clang/lib/CodeGen/CodeGenModule.cpp
4639

I'd guess oversight; it's originally a gcc extension, and using it to insert padding into arrays doesn't really make sense.

We found another case where the assertion fails.

$ cat test.cpp

struct S {
  int len;
  int i[];
};

S value{0, 0};

$ clang++ -std=c++11 -c test.cpp

CstSize is only 4 while VarSize is 8.

The assertion also fails when the following code is compiled:

struct S { int i[]; };
S value{0};

According to C99, flexible array members have to belong to structs that have more than one named member. But clang allows that as a GNU extension although gcc seems to reject it.

The assertion is correct; without it, your code is getting miscompiled.

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

Looks like Init's type is wrong. It should be { i32, [1 x i32] } instead of { i32, [0 x i32] } in the case where S has two members.

efriedma added a comment.EditedMay 22 2023, 3:51 PM

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

I wasn't trying to suggest our behavior is correct here, just that the assertion is correctly indicating an underlying issue with the generated IR.

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

I wasn't trying to suggest our behavior is correct here, just that the assertion is correctly indicating an underlying issue with the generated IR.

Ah, sorry! I misunderstood. :-)