Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
AnastasiaStulova opened this issue Nov 15, 2018 · 8 comments
Labels
bugzilla Issues migrated from bugzilla OpenCL

Comments

@AnastasiaStulova
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@AnastasiaStulova
Copy link
Contributor Author

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

@zmodem
Copy link
Collaborator

zmodem commented Feb 12, 2020

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

Yes. Pushed to 10.x as b33830a.

@AnastasiaStulova
Copy link
Contributor Author

Thanks!

@zmodem
Copy link
Collaborator

zmodem commented Nov 27, 2021

mentioned in issue #43900

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla OpenCL
Projects
None yet
Development

No branches or pull requests

3 participants