[PATCH] D133574: [C2x] reject type definitions in offsetof

[YingChi Long via Phabricator via cfe-commits]

inclyc added inline comments.


================
Comment at: clang/test/Sema/offsetof.c:79
+    int a;
+    struct B // no-error, struct B is not defined within __builtin_offsetof directly
+    {
----------------
aaron.ballman wrote:
> inclyc wrote:
> > inclyc wrote:
> > > aaron.ballman wrote:
> > > > inclyc wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this is defensible. The wording in the standard is "If the specified type defines a new type or if the specified member is a bit-field, the behavior is undefined." and the specified type in this case is `struct A`; that `struct A` happens to also define `struct B` is immaterial.
> > > > > > 
> > > > > > However, the intent behind the change to the rule is to support older implementations of `offsetof` to protect them from having to deal with a case like: `offsetof(struct S { int a, b }, b);` where `offsetof` is a macro and thus the comma between `a` and `b` is treated as a separator. So there's a part of me that wonders if we want to also support diagnosing this case. But then we'd have to look at the declarator context more recursively to see whether any of the contexts on the stack are an `offsetof` context and that might be tricky.
> > > > > > 
> > > > > > Thoughts?
> > > > > FWIW, gcc seems just rejects all definitions in this context. (Perhaps during Parsing the statements). If we add a bool state to the Parser (just using RAII object as before) struct B will trigger diagnostic error because the state "ParsingOffsetof" is passed into inner declaration.
> > > > GCC accepts currently: https://godbolt.org/z/oEvzjW6Ee but you're correct regarding switching back to an RAII object being an easier way to address the nested declarations.
> > > > 
> > > > Let me think on this situation a bit....
> > > > GCC accepts currently
> > > 
> > > C++: https://godbolt.org/z/fon8e7dzf 
> > ```
> > <source>: In function 'int main()':
> > <source>:3:3: error: types may not be defined within '__builtin_offsetof'
> >     3 |   {
> >       |   ^
> > <source>:6:5: error: types may not be defined within '__builtin_offsetof'
> >     6 |     {
> >       |     ^
> > Compiler returned: 1
> > ```
> C++ is a different language in this case though. In C, you can generally define types anywhere you can spell a type, and in C++ you cannot. e.g., `void func(struct S { int x, y; } s);` is valid in C and invalid in C++.
GCC explicitly reject type definitions since GCC 8 (in C++ mode). I guess the logic here in GCC may also add a boolean state parameter to parser.

Interestingly, in C++ we treat the first parameter of `__builtin_offsetof` as a type specifier, rejecting the definition of `struct A`, but not rejecting nested definition `struct B`

https://godbolt.org/z/PqWjzqqYn

Emm, so, how about switch back to RAIIObject to support diagnosing nested definitions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133574/new/

https://reviews.llvm.org/D133574