[Clang] [Sema] Improve support for __restrict-qualified member functions by Sirraide · Pull Request #83855 · llvm/llvm-project (original) (raw)

The exact semantics of __restrict when applied to member functions are somewhat nebulous seeing as it is a compiler extension, and as a result, there are quite a few edge cases that our implementation currently doesn’t handle very well (or at all). This pr is mainly about cases such as:

struct S { void a() __restrict; };

void S::a() { }

Both GCC and MSVC support this extension, and so does Clang; the problem is that our support for it is quite incomplete in some cases. Fortunately, GCC and MSVC agree on surprisingly many things here, but there are still enough places where they differ and which we’re not handling properly at the moment.

At the same time, it should be noted that MSVC’s documentation surprisingly states that ‘The __restrict keyword is valid only on variables’, despite the fact that MSVC seems to support it on functions anyway (to the point where it affects how function names are mangled). The remainder of this discussion assumes that it is indeed supported and that the documentation is incomplete or incorrect.

__restrict in a declaration vs definition

First, both GCC and MSVC allow a mismatch in __restrict-qualification between the declaration and definition of a member function:

struct S { void a() __restrict; void b(); };

// Both of these are accepted. void S::a() { } void S::b() __restrict { }

Unfortunately, that is where the similarities end; GCC only seems to care about __restrict being present on the definition; I’ve already opened a GCC bug report for that to ask for clarification whether this is actually intended. Update: It seems that my interpretation of this was just wrong: GCC handles __restrict in this position just like any other cv-qualifier on a function parameter—i.e. it only matters if it’s specified in the definition; the confusing part about this to me was that it’s syntactically in the same position as const and volatile, but that’s about the only thing it has in common with those two. So yes, this behaviour definitely makes sense then if you look at it that way.

MSVC does the opposite, to the point where the function name is mangled differently depending on whether __restrict is present on the declaration (__restrict on an out-of-line definition does not impact mangling at all and seems to be ignored entirely; see the section on this below). The fact that it has an effect on mangling seems to indicate that ignoring __restrict if it is only present on a function definition is intended, as you’d run into linker errors otherwise.

We currently do not support this at all. The best approach to fixing this currently seems to be simply ignoring __restrict when checking if member function types are compatible.

Also, to make talking about this a bit easier, let’s call a member function ‘effectively __restrict’,

__restrict member function pointers

This also has consequences on how __restrict-qualification is handled in pointers to member functions: both GCC and MSVC basically ignore __restrict in a pointer to member type entirely:

struct S { void a() __restrict; void b(); };

// '__restrict' is effectively ignored here. static_assert(is_same_v< void (S::)() __restrict, void (S::)()

);

static_assert(is_same_v< Template<void (S::*)() __restrict>, Template<void (S::*)() >

);

// All of these are allowed. void (S::*p1)() __restrict = &S::a; void (S::*p2)() = &S::a; void (S::*p3)() __restrict = &S::b; void (S::*p4)() = &S::b;

At least with GCC, this behaviour seems intended, as its documentation explicitly states that:

As with all outermost parameter qualifiers, __restrict__ is ignored in function definition matching. This
means you only need to specify __restrict__ in a function definition, rather than in a function prototype
as well.

Expectedly, __is_same also returns true in these cases in GCC (MSVC does not have __is_same). To support this behaviour, we should strip __restrict from the type of a pointer to member function; I’ve been talking this over with @AaronBallman for quite a bit, and we both agree that attempting to instead keep it and adjust all places where it might matter to ignore it seems infeasible.

However, we cannot simply strip __restrict from all function types because of another annoying detail that GCC and MSVC also both agree on: __restrict on regular function types does matter:

// '__restrict' is not ignored here. static_assert(not is_same_v< void () __restrict, void ()

);

static_assert(not is_same_v< Template<void () __restrict>, Template<void () >

);

Fortunately, what seems to be working quite well is stripping __restrict from the function type whenever we create a MemberPointerType—this does mean we lose out on source fidelity a bit though—and ignoring __restrict checking if two function types are compatible for the purpose of merging function declarations.

Issues with other approaches

Aaron and I considered the possibility of sometimes stripping __restrict from the canonical type of a function type, but I ran into two issues with that:

  1. We’d have to make this change in getFunctionTypeInternal(), which has no idea whether we’re about to use this thing in a MemberPointerType, so doing this would entail updating every last use of getFunctionType(), which would not only be a lot of work, but also very error-prone.
  2. I have not actually checked this, but I believe the way we cache types makes this approach infeasible: Consider for instance some member function void f() __restrict, for which we then build the type void () __restrict; let’s call that type (A); its canonical type, by this current approach, would be void f(). Now, what happens if we encounter, say using X = void () __restrict. This will try to construct void () __restrict, which is the same type as (A)! Since it already exists, we reuse (A), but oh no, its canonical type doesn’t have __restrict even though it should in this case...

Another approach I considered but was quick to discard as well was always stripping __restrict from the function type of a CXXMethodDecl and tracking it as a bit in the CXXMethodDecl itself; however, every place that checks whether a CXXMethodDecl is __restrict would have to be updated as well, which is probably doable, but as Aaron also pointed out to me when I suggested that, we’d rather not have both an ‘is __restrict’ bit on the function type as well as an ‘is actually __restrict’ bit in CXXMethodDecl (or its type) if we can avoid it, because keeping those two in sync (or rather, remembering to use the latter as the former would basically never be set under that approach) sounds like it would only make things even more confusing than they already are.

In conclusion, stripping it when creating a MemberPointerType only and keeping it on the type of a CXXMethodDecl and just manually ignoring it in cases where other compilers ignore it seems to be the most feasible approach.

this

Note that __restrict differs from const and volatile in that same position because, whereas the latter two apply to the object pointed to by this—i.e. decltype(this) would be const S* in a const member function of S—__restrict, on the other hand, is applied to the this pointer itself, i.e. decltype(this) is S* __restrict (not __restrict S*):

struct S { void a() __restrict; };

// Everyone agrees that this is __restrict and that this is S* __restrict. void S::a() __restrict { static_assert(is_same_v< decltype(this), S* __restrict >); }

We recently merged a pr (#83187) that fixed some bugs regarding this.

Both compilers also agree that this is __restrict iff the member function is effectively __restrict; the rest of the examples below apply to code inside an effectively __restrict member function.

Unfortunately, lambdas make this a bit more complicated: if this is not captured by value, then it still points to the same object, and it stands to reason that the captured this would still be __restrict. GCC agrees with this and so does MSVC (with a caveat, see below):

// GCC and MSVC both agree that this is __restrict. this { static_assert(is_same< decltype(this), S* __restrict >::value); }

If we capture this by value, the situation is different; now, this refers to a different object, which means that this this is a different pointer, which would imply that we should no longer consider it __restrict; GCC agrees with that; however, MSVC still thinks it is __restrict:

*this { // Fails with GCC; passes with MSVC. static_assert(is_same< decltype(this), S* __restrict >::value); };

I mentioned a caveat with MSVC, and you may have noticed that I switched to using is_same<>::value instead of is_same_v<> for these last two assertions; this is because, inside a lambda that captures this (by value or by reference), MSVC seems to disagree with itself whether or not this is __restrict:

this { is_same_v<decltype(this), S* __restrict>; // is false is_same <decltype(this), S* __restrict>::value; // is true };

If this is not an MSVC bug, then I don’t know what it is, candidly. The reason I said earlier that MSVC does seem to consider this to be __restrict (in both cases) is because attempting to instantiate an incomplete template gives this error (a failing static_assert unfortunately only prints ‘static assertion failed’ with no extra information with MSVC, necessitating this workaround):

template struct Incomplete;

// Within a lambda that captures this or *this, MSVC gives: // error C2079: 'foo' uses undefined struct 'Incomplete<S *__restrict >' Incomplete<decltype(this)> foo;

Additionally, there is another MSVC issue I’ve run into: for the life of me, I cannot get decltype(&S::a) (where S::a is __restrict) to compare equal to any other type; the Incomplete trick above gives
void (__cdecl S::* )(void), but even that does not work. I suspect this is another MSVC bug.

Additional notes

Templates and explicit template specialisations behave just as though they were regular member functions wrt __restrict in both GCC and MSVC. Furthermore, the presence of other qualifiers (e.g. const or &&) does not seem to have any impact on the behaviour of __restrict.

This entire analysis is based on what does and does not compile, or what types are or aren’t equal. I have not taken into consideration how the presence of __restrict does or does not impact codegen in either GCC or MSVC, and I don’t believe there is a good reason to do that, seeing as this is mainly about ensuring that we agree with GCC/MSVC on what type something is.

That is, whether we actually do anything based on whether e.g. __restrict is only present on the declaration or definition is up to us, and we don’t necessarily need to agree with other compilers here. After all, it’s not like we’re ever just conjuring a __restrict out of thin air: if codegen ends up being different because we think a function is __restrict, then that means a user put __restrict somewhere, and if they didn’t want it to be __restrict, they shouldn’t have written __restrict anywhere in the first place. This pr is entirely about ensuring that code that compiles with GCC or MSVC compiles and does something valid when compiled with Clang.

Related issues wrt __restrict

MSVC (but not GCC) allows constructors and destructors to be marked as __restrict (see #27469). I would argue we should at least support this in MSVCCompat mode, but I’m not opposed to also just supporting it in general (note that, although cv-qualifiers are not allowed on constructors/destructors, I would argue that that is because it just doesn’t make sense for an object that is being constructed/destroyed to be e.g. const; however, __restrict is different because it applies to this, and not *this, so there may be situations where there might be value in making it __restrict).

As an aside, since we’re already talking about __restrict, MSVC allows a T*& to bind to a T* __restrict (see #27383 and maybe #63662), i.e. the following code is accepted by MSVC:

int* __restrict x; int*& y = x; // alternatively: int** y = &x;

GCC and Clang both reject this, and it seems semantically wrong to allow this (GCC and Clang also both complain about this in C, where restrict is actually part of the standard). There could be a case for supporting this in MSVCCompat mode (maybe downgraded to a warning as in C), but I’d leave it as is otherwise (and maybe close the issue(s) in question as wontfix in that case). This should also be a separate pr, if we do decide to change something here; I just wanted to bring this up because, if I’m already improving support for __restrict, I might as well look into other issues with it.

Effects on existing behaviour

Most of the GCC-specific behaviour for this has already been implemented by #83187; I’ve refactored the code a bit and added support for MSVC-specific behaviour.

Currently, Clang does not support a mismatch in __restrict-qualification between the declaration and definition of a member function, which means that allowing that and implementing its semantics properly (‘properly’ as far as we can tell, anyway; all of this is awfully ill-documented...) shouldn’t cause any problems.

One thing to note is that stripping __restrict in some cases definitely impacts source fidelity; I have yet to look into possible solutions or workarounds for that, but also, I personally would argue that that is less important than getting the semantics of __restrict right in the first place.

To-do list

Supporting GCC’s and MSVC’s semantics wrt __restrict seems to entail:

I’ve already implemented the points above; fortunately, I haven’t run into any major issues with the approach detailed above.

This fixes #11039.