This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix crash for sizeof on VLAs
ClosedPublic

Authored by pmatos on Dec 15 2016, 1:30 AM.

Details

Summary

Adds overload of TransformToPotentiallyEvaluated for TypeSourceInfo to
properly deal with VLAs in nested calls of sizeof and typeof. Fixes
PR31042 (https://github.com/llvm/llvm-project/issues/30390).

Diff Detail

Event Timeline

pmatos updated this revision to Diff 81547.Dec 15 2016, 1:30 AM
pmatos retitled this revision from to Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo.
pmatos updated this object.
pmatos added a reviewer: efriedma.
pmatos added a subscriber: cfe-commits.

@efriedma Here's the new patch, thanks for your help getting here.

Can you add a test case?

efriedma requested changes to this revision.Jan 9 2017, 12:46 PM
efriedma edited edge metadata.

Missing regression test in test/SemaCXX/.

This revision now requires changes to proceed.Jan 9 2017, 12:46 PM

Apologies for the delay over the holiday season, I will look into this later on after office hours.

Has this been fixed upstream already?

Ah no, my mistake. I had the patch applied when I ran the test.

pmatos updated this revision to Diff 85328.EditedJan 22 2017, 11:32 PM
pmatos edited edge metadata.

Here's an updated patch including the test. Hope this is now ok for submission. Please accept my apologies with regards to the delay in submitting this.

efriedma added inline comments.Jan 23 2017, 12:58 PM
lib/Sema/SemaExpr.cpp
4031 ↗(On Diff #85328)

Is the isUnevaluatedContext() check here necessary?

test/SemaCXX/pr31042.cpp
1 ↗(On Diff #85328)

-emit-obj ?

3 ↗(On Diff #85328)

Is this declaration necessary somehow?

pmatos marked 3 inline comments as done.Mar 14 2017, 1:54 AM

Thanks for the comments.

pmatos added inline comments.Mar 14 2017, 1:55 AM
lib/Sema/SemaExpr.cpp
4031 ↗(On Diff #85328)

Not really. Reran tests to ensure removing it doesn't break anything.

test/SemaCXX/pr31042.cpp
1 ↗(On Diff #85328)

Not needed. Left over from copying another test.

3 ↗(On Diff #85328)

Same. Not needed. Left over from copying another test.

pmatos updated this revision to Diff 91684.Mar 14 2017, 1:58 AM

@efriedma I have uploaded a new patch taking your comments into consideration and rebased on most recent clang sources.

pmatos updated this revision to Diff 91685.Mar 14 2017, 2:00 AM

Added missing testcase to previous patch.

efriedma added inline comments.Mar 15 2017, 11:43 AM
test/SemaCXX/pr31042.cpp
1 ↗(On Diff #91685)

Oh, this testcase doesn't actually crash on trunk without at least -emit-llvm because semantic analysis doesn't actually verify the used bit. :( Better to include that, I think.

pmatos updated this revision to Diff 91980.Mar 16 2017, 2:51 AM
pmatos marked an inline comment as done.
efriedma added inline comments.Mar 16 2017, 10:47 AM
test/SemaCXX/pr31042.cpp
1 ↗(On Diff #91980)

You need to use "-o -" or something like that to avoid generating a file pr31042.ll. Also, a comment explaining why this test is using -emit-llvm would be nice.

pmatos updated this revision to Diff 398935.Jan 11 2022, 7:00 AM

After a long hiatus on this bug, this is still failing on HEAD so lets get it fixed.

Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 7:00 AM
Herald added a subscriber: wingo. · View Herald Transcript
This comment was removed by pmatos.
test/SemaCXX/pr31042.cpp
1 ↗(On Diff #91685)

Ah, that's why I had initially -emit-obj. That also triggered the problem. I obviously forgot about this and removed the flag without retesting to check if still broke trunk. Apologies for that.

1 ↗(On Diff #91685)

And we also need to remove -fsyntax-only. Submitting new patch.

@efriedma I know it has been a long time, but are you still able to review this?

efriedma accepted this revision.Jan 11 2022, 11:45 AM

Looks like all the review comments have been addressed. LGTM

This revision is now accepted and ready to land.Jan 11 2022, 11:45 AM

Looks like all the review comments have been addressed. LGTM

Thanks for the review, but unfortunately I found an issue right before committing, taking a look at it now.

pmatos updated this revision to Diff 399320.Jan 12 2022, 7:08 AM
pmatos retitled this revision from Add overload of TransformToPotentiallyEvaluated for TypeSourceInfo to [clang] Fix crash for sizeof on VLAs.
pmatos edited the summary of this revision. (Show Details)

Ensure that we only call transform on Unevaluated Contexts, avoids the failure of a couple of vla tests.

Looks like all the review comments have been addressed. LGTM

Minor change.

This revision was landed with ongoing or failed builds.Jan 12 2022, 7:11 AM
This revision was automatically updated to reflect the committed changes.