Skip to content

[clang] SemaOverload Qualifiers::isAddressSpaceSupersetOf called with wrong order of arguments #64020

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.

Sign up for GitHub

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

Jump to bottom
Closed
HoBoIs opened this issue Jul 21, 2023 · 3 comments
Closed

[clang] SemaOverload Qualifiers::isAddressSpaceSupersetOf called with wrong order of arguments #64020

HoBoIs opened this issue Jul 21, 2023 · 3 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@HoBoIs
Copy link
Contributor

HoBoIs commented Jul 21, 2023 •
edited
Loading

in clang/lib/Sema/SemaOverload.cpp clang::isBetterOverloadCandidate line 10243,10245 Qualifiers::isAddressSpaceSupersetOf is called with the same arguments (AS2, AS1), making the 2nd call unnecessary. I think the order of the parameters are meant to be swapped in one of them (I think in the 2nd one). This can lead to a future bug if Qualifiers::isAddressSpaceSupersetOf (AS1,AS2) is true and an other check is added later after line 10248.
@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Jul 21, 2023
@llvmbot
Copy link
Member

llvmbot commented Jul 21, 2023

@llvm/issue-subscribers-clang-frontend

@HoBoIs
Copy link
Contributor Author

HoBoIs commented Aug 21, 2023 •
edited
Loading

@OleStrohm @AnastasiaStulova #49673

@HoBoIs HoBoIs mentioned this issue Sep 15, 2023
Merged
@AnastasiaStulova
Copy link
Contributor

Good spot! I agree that the second if statement with isAddressSpaceSupersetOf is incorrect and the address spaces should be swapped there indeed.

Fee free to create PR and I am happy to help reviewing. Maybe the OpenCL example from the bug you have linked here could be used as a test case.

HoBoIs pushed a commit to HoBoIs/llvm-project that referenced this issue Sep 15, 2023
Choose the correct deduction guide
258462c 
If there are two guides, one of them generated from a non-templated constructor
and the other from a templated constructor, then the standard gives priority to
the first. Clang detected ambiguity before, now the correct guide is chosen.

As an unrelated minor change, fix the issue llvm#64020, which could've led to
incorrect behavior if further development inserted code after a call to
isAddressSpaceSubsetOf() which specified the two parameters in the wrong order.
@whisperity whisperity changed the title SemaOverload Qualifiers::isAddressSpaceSupersetOf called with wrong order of arguments [clang] SemaOverload Qualifiers::isAddressSpaceSupersetOf called with wrong order of arguments Oct 4, 2023
erichkeane pushed a commit that referenced this issue Oct 4, 2023
@HoBoIs
[clang] Choose non-templated ctor as deduction guide unambiguously (#…
66c1916 
…66487)

If there are two guides, one of them generated from a non-templated
constructor
and the other from a templated constructor, then the standard gives
priority to
the first. Clang detected ambiguity before, now the correct guide is
chosen.
The correct behavior is described in this paper:
http://wg21.link/P0620R0

Example for the bug: http://godbolt.org/z/ee3e9qG78

As an unrelated minor change, fix the issue
#64020,
which could've led to incorrect behavior if further development inserted
code after a call to
`isAddressSpaceSubsetOf()`, which specified the two parameters in the
wrong order.

---------

Co-authored-by: hobois <[email protected]>
@HoBoIs HoBoIs closed this as completed Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@EugeneZelenko @AnastasiaStulova @HoBoIs @llvmbot