This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Fix clang crash on aggregate initialization of array of labels
ClosedPublic

Authored by johannes on Nov 15 2019, 3:35 AM.

Details

Summary

Fix PR43700

The ConstantEmitter in AggExprEmitter::EmitArrayInit was initialized
with the CodeGenFunction set to null, which caused the crash.
Also simplify another call, and make the CGF member a const pointer
since it is public but only assigned in the constructor.

Diff Detail

Event Timeline

johannes created this revision.Nov 15 2019, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2019, 3:35 AM
nickie marked an inline comment as done.Nov 15 2019, 4:45 AM
nickie added a subscriber: nickie.

LGTM, for all that it's worth.

clang/lib/CodeGen/CGExpr.cpp
1021 ↗(On Diff #229497)

I don't think that const is necessary here. We're talking about the function's parameter. Why isn't E a *const too?

johannes updated this revision to Diff 229556.EditedNov 15 2019, 8:09 AM

Remove unnecessary const on parameter

johannes marked an inline comment as done.Nov 15 2019, 8:12 AM
johannes added inline comments.
clang/lib/CodeGen/CGExpr.cpp
1021 ↗(On Diff #229497)

Right, this was left over from when I thought it was necessary. Removed the const because it is surprising.

johannes updated this revision to Diff 231330.Nov 27 2019, 4:06 PM
johannes edited the summary of this revision. (Show Details)

update commit message

This revision was not accepted when it landed; it landed in state Needs Review.Nov 27 2019, 4:17 PM
This revision was automatically updated to reflect the committed changes.

Committed as 1ac700cdef787383ad49a0e37d9894491ef19480 - this should be a safe fix

I think this was commited too soon. You should receive LGTM from clang maintainer(s).

LGTM, for all that it's worth.

rnk added a comment.Nov 28 2019, 6:46 AM

Fix looks good post commit, but we should enhance the test.

clang/test/CodeGen/label-array-aggregate-init.c
1

It's best practice to filecheck for something, even if this used to crash.

In D70302#1763025, @rnk wrote:

Fix looks good post commit, but we should enhance the test.

I see, apologies for the premature commit.

clang/test/CodeGen/label-array-aggregate-init.c
1

I guess the test should make sure that the array is constructed successfully after codegen.
I'm unsure about the best way to test that.

rnk added a comment.Dec 2 2019, 3:07 PM

I see, apologies for the premature commit.

No problem, LLVM's code review practices are somewhat opaque.

clang/test/CodeGen/label-array-aggregate-init.c
1

I went ahead and did it in rG536cedaecbe586ec9cf86d5102872adc27e6ea23.