This is an archive of the discontinued LLVM Phabricator instance.

[clang][TargetInfo] Use LangAS for getPointer{Width,Align}()
ClosedPublic

Authored by arichardson on Nov 18 2022, 7:09 AM.

Details

Summary

Mixing LLVM and Clang address spaces can result in subtle bugs, and there
is no need for this hook to use the LLVM IR level address spaces.
Most of this change is just replacing zero with LangAS::Default,
but it also allows us to remove a few calls to getTargetAddressSpace().

This also removes a stale comment+workaround in
CGDebugInfo::CreatePointerLikeType(): ASTContext::getTypeSize() does
return the expected size for ReferenceType (and handles address spaces).

Diff Detail

Event Timeline

arichardson created this revision.Nov 18 2022, 7:09 AM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
arichardson requested review of this revision.Nov 18 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2022, 7:09 AM

clang-format

I could also use {} instead of LangAS::Default, but the latter seems more obvious

This looks great, thanks! Skimmed through the changes pretty quickly.

clang/lib/AST/Mangle.cpp
237

Since you're changing this, please hoist this call out so that we do it once.

clang/lib/AST/RecordLayoutBuilder.cpp
1915

This is a nice cleanup, but I actually can't figure out why we can't just fall into the code below.

clang/lib/CodeGen/CGDebugInfo.cpp
1706

I'm pretty sure we have language modes that support methods in different address spaces, and your code doesn't seem to require this assertion.

arichardson marked 3 inline comments as done.

address review feedback

arichardson added inline comments.Nov 30 2022, 4:04 AM
clang/lib/AST/RecordLayoutBuilder.cpp
1915

It looks like this dates back to Add a new ASTRecordLayoutBuilder class. Not used yet. from 2009. Maybe ReferenceTypes were not handled correctly by Context.getTypeInfoInChars(RT); back then?

1915

I've removed the special case now and it looks like all tests are passing, so I believe this is indeed no longer needed.

clang/lib/CodeGen/CGDebugInfo.cpp
1706

I added this assertion to see if we have any tests for this case and forgot to remove it. I think it should hopefully be correct, so I've removed the assertion.

rjmccall accepted this revision.Nov 30 2022, 10:22 AM

LGTM

clang/lib/AST/RecordLayoutBuilder.cpp
1915

Thanks!

This revision is now accepted and ready to land.Nov 30 2022, 10:22 AM
jrtc27 added inline comments.Nov 30 2022, 10:27 AM
clang/lib/AST/ASTContext.cpp
2228–2231

clang-format(?) screwed up this comment, maybe better to just put it all on one line before these rather than as inline?

This revision was automatically updated to reflect the committed changes.
arichardson marked an inline comment as done.
arichardson added inline comments.Nov 30 2022, 12:27 PM
clang/lib/AST/ASTContext.cpp
2228–2231

Thanks, fixed now.