Page MenuHomePhabricator

Sema: Fix explicit address space cast in C++

Authored by yaxunl on Jul 13 2018, 7:17 AM.



Currently clang does not allow implicit cast of a pointer to a pointer type in different address space but allows C-style cast of a pointer to a pointer type in different address space. However, there is a bug in Sema causing incorrect Cast Expr in AST for the latter case, which in turn results in invalid LLVM IR in codegen.

This is because Sema::IsQualificationConversion returns true for a cast of pointer to a pointer type in different address space, which in turn allows a standard conversion and results in a cast expression with no op in AST.

This patch fixes that by let Sema::IsQualificationConversion returns false for a cast of pointer to a pointer type in different address space, which in turn disallows standard conversion, implicit cast, and static cast. Finally it results in an reinterpret cast and correct conversion kind is set.

Diff Detail


Event Timeline

yaxunl created this revision.Jul 13 2018, 7:17 AM
rjmccall added inline comments.Jul 17 2018, 3:27 PM
2171 ↗(On Diff #155353)

Please extract this to a separate function so you can reuse intermediate results.

And in the presence of address-space promotions, you need to do a promotion followed by a reinterpretation, right?

3150 ↗(On Diff #155353)

It's not really OpenCL C++ that's special here, it's the possibility of promotions between address spaces.

yaxunl added inline comments.Jul 18 2018, 9:07 AM
3150 ↗(On Diff #155353)

For OpenCL C++, there is language rule about address space promotion.

For other languages, there is no generic rule about adderss space promotion, since the meaning of address space and their relations are target dependent. Do we want to introduce a target hook Target::canPromote(AddressSpace Src, AddressSpace Dest, LanguageOptions LangOpts) to represent this? Or do we just assume a simple rule, that is, all address space can be promoted to default address space 0, otherwise it is not allowed?

rjmccall added inline comments.Jul 18 2018, 10:47 AM
3150 ↗(On Diff #155353)

A target-specific hook for handling target-specific address spaces makes sense to me. I don't think there's a generic rule allowing promotion into the default AS.

yaxunl marked an inline comment as done.Jul 19 2018, 11:43 AM
yaxunl added inline comments.
3150 ↗(On Diff #155353)

I checked the current logic about address space promotions. There are several functions Type::Qualifier::isAddressSpaceSupersetOf, Type:;Qualifier::compatiblyIncludes, PointerType::isAddressSpaceOverlapping which checks address space compatibility. However they are only based on language rules and do not depend on targets. Since only OpenCL defined language rules regarding address spaces, address space promotion is only allowed for OpenCL.

To allow target specific address space promotion, these functions need an extra ASTContext parameter to allow them access LanguageOptions and TargetInfo. I am not sure if this makes things overly complicated.

I would expect target specific address space casts in C++ to be rare cases since it is not part of language standard. In most cases users would just use generic pointer. When they do use explicit address space cast, I expect they understand what they are doing. Then clang just do not do any target-specific promotion and treat it as reinterpret cast. Promotion is only allowed by language rules e.g. those defined by OpenCL.

rjmccall added inline comments.Jul 19 2018, 12:31 PM
3150 ↗(On Diff #155353)

Well, address spaces in "pure" C/C++ are specified by ISO/IEC TR 18037, the Embedded C specification, which does cover the possibility of target-specific overlapping address spaces. Since we don't support those yet, that's fine, I'm not going to ask you to add that infrastructure; but I do think you should at least handle the language-specific overlap rules here, and in particular you should handle them by calling one of those functions you mentioned, so that if someone *does* want to implement target-specific overlapping address spaces, it's obvious that this is one of the places that needs to be fixed and tested.

yaxunl updated this revision to Diff 156403.Jul 19 2018, 5:12 PM
yaxunl marked 5 inline comments as done.

Revised by John's comments.

rjmccall accepted this revision.Jul 19 2018, 6:25 PM

Thanks, looks good.

This revision is now accepted and ready to land.Jul 19 2018, 6:25 PM
This revision was automatically updated to reflect the committed changes.