Consider forbidding non-constant expressions in `offsetof`

[trofi]

Bugzilla Link	48407
Version	unspecified
OS	Linux
CC	@AaronBallman,@DougGregor,@efriedma-quic,@zygoloid

Extended Description

gcc-11 will deliberately become stricter and will disallow non-constant expressions in offsetof: https://gcc.gnu.org/#95942

Complete example:

  // $ cat.bug.cc
  struct a {
    int b;
    char c[100];
  };
  unsigned long d(long e) { return __builtin_offsetof(a, c[e]); }

$ g++-11.0.0 -c bug.cc -o bug.o
bug.cc: In function 'long unsigned int d(long int)':
bug.cc:6:60: error: 'e' is not a constant expression
    6 |   unsigned long d(long e) { return __builtin_offsetof(a, c[e]); }
      |                                                            ^
$  clang++-11 -c bug.cc -o bug.o
<ok>

Should clang also reject the code above? If not should clang emit warning on -Wall that non-standard extension is being used?

Thank you!

[zygoloid]

This code does not constitute a constraint violation, and it seems unhelpful to reject it. Warning on it does seem reasonable, though, as it's not strictly-conforming -- especially since recent GCC rejects.

[AaronBallman]

This code does not constitute a constraint violation, and it seems unhelpful
to reject it. Warning on it does seem reasonable, though, as it's not
strictly-conforming -- especially since recent GCC rejects.

I think I agree that it doesn't directly constitute a constraint violation, but I think there are times when we should reject it.

7.19p3 says (in part):

The type and member designator shall be such that given
static type t;
then the expression &(t.member-designator) evaluates to an address constant.

6.6p9 says:

An address constant is a null pointer, a pointer to an lvalue designating an object of static storage duration, or a pointer to a function designator; it shall be created explicitly using the unary & operator or an integer constant cast to pointer type, or implicitly by the use of an expression of array or function type. The array-subscript [] and member-access . and-> operators, the address & and indirection * unary operators, and pointer casts may be used in the creation of an address constant, but the value of an object shall not be accessed by use of these operators.

And 6.7.1p8 says:

If an aggregate or union object is declared with a storage-class specifier other than typedef, the properties resulting from the storage-class specifier, except with respect to linkage, also apply to the members of the object, and so on recursively for any aggregate or union member objects.

So it's not a constraint violation because the designated member has static storage duration and so it's an address constant.

However, c[e] is indexing into the array via a parameter, so I don't see how offsetof can be expected to return an integer constant expression result in all cases, even if Clang treats use of function parameters in a constant expression as an extension of constant expressions per 6.6p10. For example,

struct a {
  int b;
  char c[100];
};

void d(long e) { int array[__builtin_offsetof(a, c[e])]; }

int main(void) {
  d(rand() % 100);
}

So I think rejection (at least in this case) is reasonable rather than runtime UB. If Clang treats the function parameter as being usable in a constant expression if the argument to the call is a constant expression, then I think warning about the non-portable behavior is reasonable.

If you'd like me to ask WG14 what they think about it, I'm happy to ask on the reflectors.

[efriedma-quic]

I was under the impression that accepting variables in offsetof is an intentional extension.

I did some quick tests, and it looks like in C mode, gcc has always accepted arbitrary expressions in __builtin_offsetof, and continues to accept them on trunk. The story for C++ is more complicated: apparently, the support was incomplete, and maybe unintentional. This is what was changed recently.

If the C/C++ working groups decide this is a constraint violation, we can add a pedantic warning. I don't see any reason to do anything else; it's a widely supported extension in both C and C++.

[AaronBallman]

I was under the impression that accepting variables in offsetof is an
intentional extension.

That's perhaps a reasonable impression. The C Standard just says "member designator" without ever defining what such a thing is. I guess I've always viewed offsetof(type, array[0]) as a member designator and so it feels like offsetof(type, array[n]) also designates a member. The real question is: does it designate one such that the expression:

static type t;
&(t. member-designator)

results in an address constant.

I did some quick tests, and it looks like in C mode, gcc has always accepted
arbitrary expressions in __builtin_offsetof, and continues to accept them on
trunk. The story for C++ is more complicated: apparently, the support was
incomplete, and maybe unintentional. This is what was changed recently.

If the C/C++ working groups decide this is a constraint violation, we can
add a pedantic warning. I don't see any reason to do anything else; it's a
widely supported extension in both C and C++.

I would argue that we are required to diagnose the case where the result is not an address constant, such as the example I posted above (which we do bad things with today).

[AaronBallman]

I would argue that we are required to diagnose the case where the result is
not an address constant, such as the example I posted above (which we do bad
things with today).

Actually, I may walk that back a bit -- we may not be doing as bad of things as I had originally thought (I may have been reading the assembly we generate incorrectly).

So I'd say if we do bad things, we should reject.

[efriedma-quic]

As far as I know, the generated code has the obvious behavior. If the offsetof involves a variable, we compute it at runtime. Your example declares a VLA of size "e+4".

[AaronBallman]

As far as I know, the generated code has the obvious behavior. If the
offsetof involves a variable, we compute it at runtime. Your example
declares a VLA of size "e+4".

Yes, that's what happens from what I'm seeing. But offsetof is defined as "which expands to an integer constant expression that has type size_t" and I read that as prohibiting forming a VLA for that code.

[zygoloid]

Permitting a non-constant array index here seems like a very obvious extension, and until GCC's recent change, it was quite plausibly the case that (within experimental error) literally every C and C++ implementation provided that extension -- either as a consequence of defining offsetof as a macro for something like (size_t)&((T*)0)->designator or because they implemented __builtin_offsetof the same as GCC historically did.

Given that there is almost certainly a significant amount of real C code relying on this, and the implementation in Clang (as far as we know) works correctly according to programmer expectations (the same as the macro implementation), it seems hard to justify rejecting it, and instead I'd suggest that we embrace this as a supported Clang extension.

I think we should add an Extension or ExtWarn diagnostic for this, both to track our intent to explicitly support it as an extension, and because C++ code using this extension might not be GCC-compatible in all cases. I think it'd also be reasonable to ask WG14 whether they'd consider changing the rule.

[AaronBallman]

Permitting a non-constant array index here seems like a very obvious
extension, and until GCC's recent change, it was quite plausibly the case
that (within experimental error) literally every C and C++ implementation
provided that extension -- either as a consequence of defining offsetof as a
macro for something like (size_t)&((T*)0)->designator or because they
implemented __builtin_offsetof the same as GCC historically did.

Given that there is almost certainly a significant amount of real C code
relying on this, and the implementation in Clang (as far as we know) works
correctly according to programmer expectations (the same as the macro
implementation), it seems hard to justify rejecting it, and instead I'd
suggest that we embrace this as a supported Clang extension.

Ah, I wasn't aware of the extent to which this was a commonly used thing. I agree we shouldn't break code here.

I think we should add an Extension or ExtWarn diagnostic for this, both to
track our intent to explicitly support it as an extension, and because C++
code using this extension might not be GCC-compatible in all cases. I think
it'd also be reasonable to ask WG14 whether they'd consider changing the
rule.

I agree, that seems sensible. In terms of asking WG14 to make a change, are you thinking of requesting something along the lines of offsetof having to return an integer constant expression unless the member designator designates a variably modified type? Or do you have a different idea in mind? I'm happy to collaborate on the paper, if you'd like.

[zygoloid]

I agree, that seems sensible. In terms of asking WG14 to make a change, are you
thinking of requesting something along the lines of offsetof having to return an
integer constant expression unless the member designator designates a variably
modified type?

I'm thinking that we should allow an arbitrary array index, and offsetof(base, designator) should only be an ICE if the corresponding notional member access is an address constant expression. I didn't follow what the connection to variably modified types is.

[AaronBallman]

I agree, that seems sensible. In terms of asking WG14 to make a change, are you
thinking of requesting something along the lines of offsetof having to return an
integer constant expression unless the member designator designates a variably
modified type?

I'm thinking that we should allow an arbitrary array index, and
offsetof(base, designator) should only be an ICE if the corresponding
notional member access is an address constant expression.

offsetof is defined as using a static declaration of an object of the given type and the expression &(that_object.the_designator), and a pointer to an lvalue designating an object of static storage duration is an address constant (and aggregate members inherit the storage duration of the aggregate itself), so I'm not certain what circumstances an arbitrary array index would result in something other than an address constant expression.

I didn't follow
what the connection to variably modified types is.

My thinking is: if it's a variably modified type, the offset to the specific element may not be known until runtime, and so the offsetof macro shouldn't be required to result in an integer constant expression.

[zygoloid]

I'm not certain what circumstances an arbitrary array index would result in
something other than an address constant expression.

In the original example:

__builtin_offsetof(a, c[e]);

&(invented_object.c[e]) is not an address constant expression, because e is not a constant expression.

My thinking is: if it's a variably modified type, the offset to the specific
element may not be known until runtime

Even with variably modified types, offsets are still computable at compile time (unless you have a non-constant array subscript). In order to get non-constant offsets, you need the GCC "VLA in struct" extension, and the fact that that leads to non-constant field offsets is precisely why we think it's too horrible for Clang to ever support.

[AaronBallman]

I'm not certain what circumstances an arbitrary array index would result in
something other than an address constant expression.

In the original example:

__builtin_offsetof(a, c[e]);

&(invented_object.c[e]) is not an address constant expression, because e is
not a constant expression.

Logically, I get that, but does the C standard actually say that? 6.6p9 seems to imply that it really is an address constant, which is part of what's tripping me up here. It seems like &(invented_object.c[e]) is "an lvalue designating an object of static storage duration" in this case, and it was created explicitly by using the & operator.

I guess I'm wondering whether that definition in 6.6p9 is actually correct or if there's something else I've missed in the wording.

My thinking is: if it's a variably modified type, the offset to the specific
element may not be known until runtime

Even with variably modified types, offsets are still computable at compile
time (unless you have a non-constant array subscript). In order to get
non-constant offsets, you need the GCC "VLA in struct" extension, and the
fact that that leads to non-constant field offsets is precisely why we think
it's too horrible for Clang to ever support.

Ah, yes, I see -- you're right, there's no connection to a VM type needed to hit the case we want to correct. Sorry for that confusion.

[zygoloid]

6.6p9 seems to imply that it really is an address constant

I think 6.6p9 does cover the case in question:

6.6p9: "The array-subscript [] [...] operator[...] may be used in the creation of an address constant, but the value of an object shall not be accessed by use of these operators."

something.c[e] accesses the value of the object e, so it's not an address constant.

Disturbingly, 'something.c[1+1]' also appears to not be an address constant, because the + operator is not among those that are permitted in an address constant.

[AaronBallman]

6.6p9 seems to imply that it really is an address constant

I think 6.6p9 does cover the case in question:

6.6p9: "The array-subscript [] [...] operator[...] may be used in the
creation of an address constant, but the value of an object shall not be
accessed by use of these operators."

'something.c[e]' accesses the value of the object 'e', so it's not an
address constant.

Ohhhh! "the value of an object" (emphasis mine), was the bit I was missing. Thank you for clarifying!

Disturbingly, 'something.c[1+1]' also appears to not be an address constant,
because the + operator is not among those that are permitted in an address
constant.

Oh, I hadn't picked up on that implication because it says the list of operators may be used but doesn't actually say the other operators are excluded. Does that also prohibit something.c[sizeof(int)] from forming an address constant?