Page MenuHomePhabricator

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

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



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


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.
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.
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
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.