This is an archive of the discontinued LLVM Phabricator instance.

[clang codegen] Fix alignment of "Address" for incomplete array pointer.
ClosedPublic

Authored by efriedma on Apr 28 2020, 4:14 PM.

Details

Summary

The code was assuming all incomplete types don't have meaningful alignment, but that clearly isn't true.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45710

Diff Detail

Event Timeline

efriedma created this revision.Apr 28 2020, 4:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 4:14 PM
rsmith added a subscriber: rsmith.Apr 28 2020, 4:23 PM
rsmith added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
176 ↗(On Diff #260792)

I don't know if it matters in practice, but this is still wrong. An incomplete type can have a known alignment, for a case like struct alignas(32) S;. Perhaps we should remove this test entirely and call getTypeAlignIfKnown instead of getTypeAlign[InChars] below.

efriedma marked an inline comment as done.Apr 28 2020, 5:26 PM
efriedma added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
176 ↗(On Diff #260792)

I can't think of any way to observe the alignment computed by getNaturalTypeAlignment for an incomplete class. We usually only use the alignment computed by getNaturalTypeAlignment() to set the alignment of memory operations, and you can't do any memory operations with an incomplete class.

But the result might be easier to read, in any case.

rjmccall added inline comments.Apr 28 2020, 6:53 PM
clang/lib/CodeGen/CodeGenFunction.cpp
176 ↗(On Diff #260792)

If there's a getTypeAlignIfKnown(), it would be better to use it.

efriedma updated this revision to Diff 267994.Jun 2 2020, 2:30 PM

"Address" the review comments. Not really happy with this, but not sure what else to do.

efriedma planned changes to this revision.Jun 2 2020, 2:39 PM

Hang on, I submitted this too early. Need to look a bit more.

efriedma updated this revision to Diff 267999.Jun 2 2020, 3:06 PM

This should work correctly, now, I think?

This revision is now accepted and ready to land.Jun 3 2020, 10:42 AM
This revision was automatically updated to reflect the committed changes.