[C++] ICE on the nested pointer indirection with address space conversion

[AnastasiaStulova]

Bugzilla Link	39674
Resolution	FIXED
Resolved on	Feb 12, 2020 04:56
Version	trunk
OS	Linux
Blocks	#43900
CC	@AnastasiaStulova,@bevin-hansson,@zmodem,@dobbelaj-snps

Extended Description

Multiple pointer indirection casting isn't working correctly. I.e. the following program:

kernel void foo(){
__private int** loc;
int** loc_p = loc;
**loc_p = 1;
}
will fail with ICE in C++ mode, but for C it will generate:

bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*

and then perform store over pointer in AS 4 (generic). We have now lost the information that the original pointer was in private AS and that the adjustment of AS segment has to be performed before accessing memory pointed by the pointer. Based on the current specification of addrspacecast in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not very clear whether it can be used for this case without any modifications or clarifications and also what would happen if there are multiple AS mismatches.

C++'s rules assume that qualifiers don't introduce real representation differences and that operations on qualified types are compatible with operations on unqualified types. That's not true of qualifiers in general: address space qualifiers can change representations, ARC qualifiers can have incompatible semantics, etc. There is no way to soundly implement a conversion from __private int ** to __generic int **, just there's no way to soundly implement a conversion from Derived ** to Base **.

Following this logic it seems reasonable to just disallow such conversions in order to prevent surprising behavior in the program.

See original discussion in https://reviews.llvm.org/D53764

[bevin-hansson]

The issue in the provided example is that the 'discards qualifiers in nested pointers' diagnostic will override the 'discards qualifiers' diagnostic, even though the latter is more serious. This is seen in checkPointerTypesForAssignment:

  // Treat address-space mismatches as fatal.  TODO: address subspaces
  if (!lhq.isAddressSpaceSupersetOf(rhq))
    ConvTy = Sema::IncompatiblePointerDiscardsQualifiers;

Instead of instantly being an error, this will be overridden by IncompatibleNestedPointerQualifiers if the pointer incompatibility was due to qualifier mismatch. That assignment result doesn't take address spaces into account and will always be a warning rather than an error.

A similar issue is in SemaCast, checkAddressSpaceCast, which only verifies the top level address space.

[AnastasiaStulova]

I was wondering if we should just extend the qualification conversion rules to reject code where address spaces are converted in nested pointers? (Just like John suggested in the review I linked). I think this will avoid potential programming errors with address spaces (just like in the example of base and derived that he provided)...

[bevin-hansson]

That sounds reasonable. I can't really think of any case where you'd want to do that.

I've submitted a patch which (I think?) does this here: https://reviews.llvm.org/D58236

[AnastasiaStulova]

The fix for C++ mode: https://reviews.llvm.org/D73360

[AnastasiaStulova]

@​hans, is it still possible to merge https://reviews.llvm.org/rG6064f426a18304e16b51cc79e74c9c2d55ef5a9c in release 10 branch?

[zmodem]

@​hans, is it still possible to merge
https://reviews.llvm.org/rG6064f426a18304e16b51cc79e74c9c2d55ef5a9c in
release 10 branch?

Yes. Pushed to 10.x as b33830a.

[AnastasiaStulova]

Thanks!

[zmodem]

mentioned in issue #43900