Convert clang::LangAS to a strongly typed enum
ClosedPublic

Authored by arichardson on Oct 11 2017, 12:59 PM.

Details

Summary

Convert clang::LangAS to a strongly typed enum

Currently both clang AST address spaces and target specific address spaces
are represented as unsigned which can lead to subtle errors if the wrong
type is passed. It is especially confusing in the CodeGen files as it is
not possible to see what kind of address space should be passed to a
function without looking at the implementation.
I originally made this change for our LLVM fork for the CHERI architecture
where we make extensive use of address spaces to differentiate between
capabilities and pointers. When merging the upstream changes I usually
run into some test failures or runtime crashes because the wrong kind of
address space is passed to a function. By converting the LangAS enum to a
C++11 we can catch these errors at compile time. Additionally, it is now
obvious from the function signature which kind of address space it expects.

I found the following errors while writing this patch:

  • ItaniumRecordLayoutBuilder::LayoutField was passing a clang AST address space to TargetInfo::getPointer{Width,Align}()
  • TypePrinter::printAttributedAfter() was printing the numeric value of the clang AST address space instead of the target address space
  • initializeForBlockHeader() in CGBlocks.cpp was passing LangAS::opencl_generic to TargetInfo::getPointer{Width,Align}()
  • CodeGenFunction::EmitBlockLiteral() was passing a AST address space to TargetInfo::getPointerWidth()
  • CGOpenMPRuntimeNVPTX::translateParameter() passed a target address space to Qualifiers::addAddressSpace()
  • CGOpenMPRuntimeNVPTX::getParameterAddress() was using llvm::Type::getPointerTo() with a AST address space
  • clang_getAddressSpace() returns either a LangAS or a target address space. As this is exposed to C I have kept the current behaviour and added a comment stating that it is probably not correct.

Other than this the patch should not cause any functional changes.

Diff Detail

Repository
rL LLVM
arichardson created this revision.Oct 11 2017, 12:59 PM
jlebar added a subscriber: jlebar.Oct 11 2017, 1:26 PM

My only regret is that I have but one +1 to give to this patch.

include/clang/Basic/AddressSpaces.h
51 ↗(On Diff #118672)

I wonder if you need this namespace? LangAS right next to LanguageAS reads strangely to me -- "what's the difference?".

I guess you'd need to rename Map and fromTargetAS, but the other two members are probably OK?

arichardson added inline comments.Oct 11 2017, 2:39 PM
include/clang/Basic/AddressSpaces.h
51 ↗(On Diff #118672)

The only reason I added this namespace is that I wasn't sure whether having those functions in the clang namespace is acceptable.

Not quite sure what to call the functions though. langASFromTargetAS?

The only reason I added this namespace is that I wasn't sure whether having those functions in the clang namespace is acceptable.

Maybe someone else will object, or suggest an existing namespace they should be in. FWIW I think it's fine.

Not quite sure what to call the functions though. langASFromTargetAS?

sgtm!

Removed the additional namespace

Anastasia added inline comments.Oct 12 2017, 8:18 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Why do we need this change?

arichardson added inline comments.Oct 12 2017, 9:11 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

__attribute__((address_space(n))) is a target address space and not a language address space like LangAS::opencl_generic. Isn't Qualifiers::getAddressSpaceAttributePrintValue() meant exactly for this use case?

bader accepted this revision.Oct 12 2017, 10:20 AM

@arichardson, great work! Thanks a lot!

This revision is now accepted and ready to land.Oct 12 2017, 10:20 AM
yaxunl added inline comments.Oct 12 2017, 1:17 PM
include/clang/Basic/AddressSpaces.h
66 ↗(On Diff #118784)

how about getLangASFromTargetAS ? It is preferred to start with small letters.

tools/libclang/CXType.cpp
402 ↗(On Diff #118784)

Is this function suppose to return AST address space or target address space?

Some targets e.g. x86 maps all AST address spaces to 0. Returning target address space will not let the client unable to differentiate different address spaces in the source language.

arichardson planned changes to this revision.Oct 12 2017, 3:28 PM
arichardson added inline comments.
include/clang/Basic/AddressSpaces.h
66 ↗(On Diff #118784)

Sounds good, I'll do that.

tools/libclang/CXType.cpp
402 ↗(On Diff #118784)

I am not entirely sure what the correct return value is here because the current implementation returns either the LanguageAS or LangAS - LangAS::FirstTargetAddressSpace which can also overlap. So possibly it should just always returning the AST address space?

I think for now I will just keep the current behaviour with a FIXME and create a followup patch.

yaxunl added inline comments.Oct 12 2017, 6:31 PM
tools/libclang/CXType.cpp
402 ↗(On Diff #118784)

That's fine. Thanks.

arichardson edited the summary of this revision. (Show Details)
  • Keep old behaviour for clang_getAddressSpace()
  • renamed to getLangASFromTargetAS()
  • rebased on latest trunk
This revision is now accepted and ready to land.Oct 13 2017, 8:30 AM
Anastasia added inline comments.Oct 13 2017, 8:38 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Yes, I think there are some adjustment we do in this method to get the original source value to be printed corerctly. Does this mean we have no tests that caught this issue?

arichardson added inline comments.Oct 13 2017, 8:39 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Seems like it, all tests pass both with and without this patch.

Anastasia added inline comments.Oct 13 2017, 9:20 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Strange considering that we have this attribute printed in some error messages of some Sema tests. If I compile this code without your patch:

typedef int __attribute__((address_space(1))) int_1;
typedef int __attribute__((address_space(2))) int_2;

void f0(int_1 &); 
void f0(const int_1 &);

void test_f0() {
  int i;
  static int_2 i2;
  f0(i);
  f0(i2);
}

I get the address spaces printed correctly inside the type:

note: candidate function not viable: 1st argument ('int_2' (aka '__attribute__((address_space(2))) int')) is in address space 2, but parameter must be in address space 1

Perhaps @yaxunl could comment further on whether this change is needed.

arichardson added inline comments.Oct 13 2017, 9:26 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

My guess is that it doesn't go through that switch statement but rather through Qualifiers::print(). I'll try adding a llvm_unreachable() to see if there are any tests that go down this path.

arichardson added inline comments.Oct 13 2017, 9:41 AM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

I just ran the clang tests with an llvm_unreachable() here and none of them failed. So it seems like we don't have anything testing this code path.

yaxunl added inline comments.Oct 13 2017, 1:39 PM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Sorry for the delay. This part of code is for printing the addr space of AttributedType. Since it seems not used by any language yet, there is no test for it. It is possible a non-target-specific address space being printed here if a language chooses to use AttributedType to represent address space. Therefore a proper fix would be isolate the code for printing address space from Qualifiers::print and re-use it here so that addr space is printed in consistent way no matter it is represented as qualifier or as AttributedType.

arichardson added inline comments.Oct 13 2017, 3:18 PM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Thanks, that makes sense. To avoid breaking anything here I think it should be part of a separate patch though.

yaxunl added inline comments.Oct 13 2017, 6:36 PM
lib/AST/TypePrinter.cpp
1323 ↗(On Diff #118784)

Sure. In this one probably keep the original behavior.

yaxunl accepted this revision.Oct 14 2017, 5:08 AM

LGTM. Thanks! Great work!

This revision was automatically updated to reflect the committed changes.