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

"1st argument ('__restrict int') would lose restrict qualifier" when passing (this restrict *)->int to int& argument #82941

Closed
nabijaczleweli opened this issue Feb 26, 2024 · 5 comments · Fixed by #83187
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party

Comments

@nabijaczleweli
Copy link

nabijaczleweli commented Feb 26, 2024

void f(int& x) {
    (void)x;
}

class C {
    int x;
    void g() __restrict;
};

void C::g() __restrict {
    f(this->x);
}

(same for void f(int const& x)) compiles fine on GCC, but Clang 17 and 19.0.0 (++20240212105217+93cdd1b5cfa3-1~exp1~20240212225341.524) rejects it with

a.cpp:11:5: error: no matching function for call to 'f'
   11 |     f(this->x);
      |     ^
a.cpp:1:6: note: candidate function not viable: 1st argument ('__restrict int') would lose restrict qualifier
    1 | void f(int& x) {
      |      ^ ~~~~~~
1 error generated.

I don't think this is valid – this looks like this is of type C restrict *, and thus x is compatible with int restrict &, and it's treating restrict like a cv-qualifier, which I don't think it is (and isn't for pointers), and even if it were I haven't found a position where you could make f take an int restrict &.

@zopsicle
Copy link

zopsicle commented Feb 26, 2024

this looks like this is of type C restrict *

The following example confirms this:

class C
{
    void g() __restrict;
};

void C::g() __restrict
{
    double x = this;
}

fails with:

error: cannot initialize a variable of type 'double' with an rvalue of type '__restrict C *'
    8 |     double x = this;
      |            ^   ~~~~

note that the diagnostic says __restrict C *, not C * __restrict. Compiling the same program with GCC fails with:

error: cannot convert 'C* __restrict__' to 'double' in initialization
    8 |     double x = this;
      |                ^~~~
      |                |
      |                C* __restrict__

zopsicle pushed a commit to zopsicle/PhysX that referenced this issue Feb 26, 2024
This does not compile correctly when using Clang [1].

[1]: llvm/llvm-project#82941
zopsicle pushed a commit to zopsicle/PhysX that referenced this issue Feb 26, 2024
This does not compile correctly when using Clang [1].

[1]: llvm/llvm-project#82941
@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Feb 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 26, 2024

@llvm/issue-subscribers-clang-frontend

Author: наб (nabijaczleweli)

```cpp void f(int& x) { (void)x; }

class C {
int x;
void g() __restrict;
};

void C::g() __restrict {
f(this->x);
}

(same for `void f(int const& x)`) compiles fine on GCC, but Clang 17 and 19.0.0 (++20240212105217+93cdd1b5cfa3-1\~exp1\~20240212225341.524) rejects it with

a.cpp:11:5: error: no matching function for call to 'f'
11 | f(this->x);
| ^
a.cpp:1:6: note: candidate function not viable: 1st argument ('__restrict int') would lose restrict qualifier
1 | void f(int& x) {
| ^ ~~~~~~
1 error generated.


I don't think this is valid – this looks like `this` is of type `C restrict *`, and thus `x` is compatible with `int restrict &`, and it's treating restrict like a cv-qualifier, which I don't think it is (and isn't for pointers), and even if it were I haven't found a position where you *could* make f take an `int restrict &`.
</details>

@shafik
Copy link
Collaborator

shafik commented Feb 26, 2024

Based on gcc documentation: https://gcc.gnu.org/onlinedocs/gcc/Restricted-Pointers.html

it does not look like we are doing the right thing here but not sure.

CC @erichkeane @AaronBallman

@AaronBallman AaronBallman added the confirmed Verified by a second party label Feb 26, 2024
@AaronBallman
Copy link
Collaborator

Yeah, this AST looks wrong to me: https://godbolt.org/z/4P7a45dP1 specifically:

`-CXXThisExpr <col:11> '__restrict S *' this

That should be a restricted pointer, not a restricted pointee.

@Sirraide
Copy link
Contributor

I think the issue seems to be that we treat __restrict like const and volatile, but whereas the latter are applied to the pointee, the former should be applied to the pointer; I’m currently working on a fix for this

Sirraide added a commit that referenced this issue Feb 29, 2024
…ns properly (#83187)

When resolving the type of `this` inside a member function, we were
attaching all qualifiers present on the member function to the class
type and then making it a pointer; however, `__restrict`, unlike `const`
and `volatile`, needs to be attached to the pointer type rather than the
pointee type.

This fixes #82941, #42411, and #18121.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants