subject:"\[PATCH\] D133574\: \[C2x\] reject type definitions in offsetof"

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

2023-01-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4086450 , @stilor wrote:

>>> I posted https://reviews.llvm.org/D142723 to address this.
>>
>> and 63d6b8be6cf248a1a8800d85a11be469c6e2 
>>  should 
>> hopefully resolve it.
>
> Thank you for sorting this out so quickly!

Any time, thank you for raising the issue so quickly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-27 Thread Alexey Neyman via Phabricator via cfe-commits

stilor added a comment.

>> I posted https://reviews.llvm.org/D142723 to address this.
>
> and 63d6b8be6cf248a1a8800d85a11be469c6e2 
>  should 
> hopefully resolve it.

Thank you for sorting this out so quickly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4085681 , @aaron.ballman 
wrote:

> In D133574#4085472 , @aaron.ballman 
> wrote:
>
>> In D133574#4085372 , 
>> @aaron.ballman wrote:
>>
>>> So by my understanding, my original changes removing the extension warning 
>>> (in D40267 ) were jumping the gun because 
>>> the committee never made the change we said we'd make. So I believe this is 
>>> still an extension as far as C conformance is concerned. That said, I'll 
>>> check with the convener to see if he'd be too frustrated if I filed a CD2 
>>> comment asking for `member-designator` to be replaced with `subobject` 
>>> based on prior discussion, so maabbeee we can fix this for C2x still.
>>
>> I heard back and there's even more confusion -- we were tracking an older 
>> copy of the DR list, and there's an update that clarifies this: 
>> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're 
>> right, the array and member access extension warnings need to be removed. 
>> I'll take care of that and get it cherry picked into the Clang 16 branch.
>
> I posted https://reviews.llvm.org/D142723 to address this.

and 63d6b8be6cf248a1a8800d85a11be469c6e2 
 should 
hopefully resolve it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4085472 , @aaron.ballman 
wrote:

> In D133574#4085372 , @aaron.ballman 
> wrote:
>
>> So by my understanding, my original changes removing the extension warning 
>> (in D40267 ) were jumping the gun because 
>> the committee never made the change we said we'd make. So I believe this is 
>> still an extension as far as C conformance is concerned. That said, I'll 
>> check with the convener to see if he'd be too frustrated if I filed a CD2 
>> comment asking for `member-designator` to be replaced with `subobject` based 
>> on prior discussion, so maabbeee we can fix this for C2x still.
>
> I heard back and there's even more confusion -- we were tracking an older 
> copy of the DR list, and there's an update that clarifies this: 
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're 
> right, the array and member access extension warnings need to be removed. 
> I'll take care of that and get it cherry picked into the Clang 16 branch.

I posted https://reviews.llvm.org/D142723 to address this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4085372 , @aaron.ballman 
wrote:

> So by my understanding, my original changes removing the extension warning 
> (in D40267 ) were jumping the gun because 
> the committee never made the change we said we'd make. So I believe this is 
> still an extension as far as C conformance is concerned. That said, I'll 
> check with the convener to see if he'd be too frustrated if I filed a CD2 
> comment asking for `member-designator` to be replaced with `subobject` based 
> on prior discussion, so maabbeee we can fix this for C2x still.

I heard back and there's even more confusion -- we were tracking an older copy 
of the DR list, and there's an update that clarifies this: 
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're 
right, the array and member access extension warnings need to be removed. I'll 
take care of that and get it cherry picked into the Clang 16 branch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4084215 , @stilor wrote:

> In D133574#4083742 , @aaron.ballman 
> wrote:
>
>> In D133574#4083724 , @asbirlea 
>> wrote:
>>
>>> Following this change, the following emits warnings: 
>>> https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE
>>>
>>> Can you please fix fwd or revert until resolved?
>>
>> What do you expect to be resolved? You're passing 
>> `-Wgnu-offsetof-extensions` which is warning you about the extensions, and 
>> `-Werror` is turning them from a warning into an error, so you're getting 
>> the behavior I'd expect based on: https://reviews.llvm.org/D133574#4059845
>
> Can you please explain how these warnings are related to the N2350? From what 
> I see in N2350 text, it was only declaring new type definitions inside of 
> `offsetof` to be undefined behavior. Can you please point to the language in 
> N2350 that prohibits member access expressions like `offsetof(x, y.z)` or 
> `offsetof(x, y[1])`?
>
> In fact, a change  some time ago explicitly 
> removed the warning for what it called an "non-identifier designator (e.g. 
> `offsetof(x, a.b[c])`" in response to DR496. The language from DR496 hasn't 
> changed in the most recent publicly available draft of the standard (which 
> includes both DR496 and N2350), so why are these "non-identifier designators" 
> considered extensions now?

Thank you for pointing this out, you're right that there's definitely some 
confusion here and I didn't explain it very well.

DR496 was considered not a defect: 
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_496 and said 
"There was no discussion asserting that this extension, however, was the actual 
intent of the standard, and as such there is was no sentiment to accept these 
extensions with clarified wording. Such a change could only be made in a new 
revision of the standard.", and the current wording of the C2x standard matches 
that of the C17 (still says member-designator and not subobject).

From my notes on the discussions of this DR:

Albuquerque 2017: "Next we moved on to DR 496 which is about offsetof. There 
was an open action item on this which was not written down, but the verbal 
report was that we should be able to safely talk about subobjects. We will 
leave this in open status and look at words later."
Pittsburgh 2017: "We did hit a procedural issue when dealing with DR 496 where 
the information from a previous PCR was not carried forward from one meeting to 
the next and there was some confusion as to what the "response" really is. We 
did mention that we want the last committee response to always be the 
definitive one, rather than making people pull together the history from 
separate locations. This is the way we usually do it, and this may have just 
been a process hiccup."
London 2018: "We looked at an open DR for 496 which we elected to mark C2x."

So by my understanding, my original changes removing the extension warning (in 
D40267 ) were jumping the gun because the 
committee never made the change we said we'd make. So I believe this is still 
an extension as far as C conformance is concerned. That said, I'll check with 
the convener to see if he'd be too frustrated if I filed a CD2 comment asking 
for `member-designator` to be replaced with `subobject` based on prior 
discussion, so maabbeee we can fix this for C2x still.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-26 Thread Alexey Neyman via Phabricator via cfe-commits

stilor added a comment.

In D133574#4083742 , @aaron.ballman 
wrote:

> In D133574#4083724 , @asbirlea 
> wrote:
>
>> Following this change, the following emits warnings: 
>> https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE
>>
>> Can you please fix fwd or revert until resolved?
>
> What do you expect to be resolved? You're passing `-Wgnu-offsetof-extensions` 
> which is warning you about the extensions, and `-Werror` is turning them from 
> a warning into an error, so you're getting the behavior I'd expect based on: 
> https://reviews.llvm.org/D133574#4059845

Can you please explain how these warnings are related to the N2350? From what I 
see in N2350 text, it was only declaring new type definitions inside of 
`offsetof` to be undefined behavior. Can you please point to the language in 
N2350 that prohibits member access expressions like `offsetof(x, y.z)` or 
`offsetof(x, y[1])`?

In fact, a change  some time ago explicitly 
removed the warning for what it called an "non-identifier designator (e.g. 
`offsetof(x, a.b[c])`" in response to DR496. The language from DR496 hasn't 
changed in the most recent publicly available draft of the standard (which 
includes both DR496 and N2350), so why are these "non-identifier designators" 
considered extensions now?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-26 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D133574#4083724 , @asbirlea wrote:

> Following this change, the following emits warnings: 
> https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE
>
> Can you please fix fwd or revert until resolved?

Those both appear to be valid warnings, I don't believe there to be anything to 
'fix' or any valid reason to revert here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-26 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4083724 , @asbirlea wrote:

> Following this change, the following emits warnings: 
> https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE
>
> Can you please fix fwd or revert until resolved?

What do you expect to be resolved? You're passing `-Wgnu-offsetof-extensions` 
which is warning you about the extensions, and `-Werror` is turning them from a 
warning into an error, so you're getting the behavior I'd expect based on: 
https://reviews.llvm.org/D133574#4059845

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-26 Thread Alina Sbirlea via Phabricator via cfe-commits

asbirlea added a comment.

Following this change, the following emits warnings: 
https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-18 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4061428 , @mstorsjo wrote:

> For clarity in the review - the relanded commit ended up reverted by 
> @aeubanks in 39da55e8f548a11f7dadefa73ea73d809a5f1729 
> . The 
> relanded commit triggers failed asserts like this:
>
>   $ echo 'void c() { __builtin_offsetof(struct {int b;}, b); }' | bin/clang 
> -cc1 -emit-llvm -o /dev/null - -x c
>   clang: ../../clang/lib/AST/RecordLayoutBuilder.cpp:3286: const 
> clang::ASTRecordLayout& clang::ASTContext::getASTRecordLayout(const 
> clang::RecordDecl*) const: Assertion `!D->isInvalidDecl() && "Cannot get 
> layout of invalid decl!"' failed.
>
> I also ran into a second issue with the reapplied version of the commit:
>
>   $ echo 'int i = __builtin_offsetof(struct {int b;}, b);' | bin/clang -cc1 
> -emit-llvm -o /dev/null - -x c
>   :1:9: error: initializer element is not a compile-time constant
>   int i = __builtin_offsetof(struct {int b;}, b);
>   ^~
>   1 error generated.
>
> (This used to compile successfully before.)

Sorry for the trouble, there was a declaration being marked as invalid despite 
us *warning* on it rather than erroring on it. I've corrected it and re-landed 
in e7300e75b51a7e7d4e81975b4be7a6c65f9a8286 
 with 
additional test coverage for both of the scenarios discovered. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-18 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added a subscriber: aeubanks.
mstorsjo added a comment.

For clarity in the review - the relanded commit ended up reverted by @aeubanks 
in 39da55e8f548a11f7dadefa73ea73d809a5f1729 
. The 
relanded commit triggers failed asserts like this:

  $ echo 'void c() { __builtin_offsetof(struct {int b;}, b); }' | bin/clang 
-cc1 -emit-llvm -o /dev/null - -x c
  clang: ../../clang/lib/AST/RecordLayoutBuilder.cpp:3286: const 
clang::ASTRecordLayout& clang::ASTContext::getASTRecordLayout(const 
clang::RecordDecl*) const: Assertion `!D->isInvalidDecl() && "Cannot get layout 
of invalid decl!"' failed.

I also ran into a second issue with the reapplied version of the commit:

  $ echo 'int i = __builtin_offsetof(struct {int b;}, b);' | bin/clang -cc1 
-emit-llvm -o /dev/null - -x c
  :1:9: error: initializer element is not a compile-time constant
  int i = __builtin_offsetof(struct {int b;}, b);
  ^~
  1 error generated.

(This used to compile successfully before.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman closed this revision.
aaron.ballman added a comment.

Closing as completed by f1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

I've relanded this as f1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee 
, changes 
include:

- Removed error in C mode, turned it into an extension warning
- Added an extension warning for using member access or array subscript 
expressions for the member designator
- Corrected expected test output
- Added documentation

The new warnings are both under a new flag `-Wgnu-offsetof-extensions` which is 
nested under the `-Wgnu` flag.

Marking this review as accepted so I can close it now that I think it's 
completed again. However, if anyone spots further concerns, please raise them!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-16 Thread Yingchi Long via Phabricator via cfe-commits

inclyc added a comment.

On Mon, Jan 16, 2023 Roman Lebedev  wrote:

> Reminder to please always mention the reason for the revert/recommit in the 
> commit message.

Thanks! This change needs to be fixed up (see comments above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-16 Thread Florian Hahn via Phabricator via cfe-commits

fhahn added a comment.

In D133574#4053647 , @aaron.ballman 
wrote:

> I'll take care of fixing this up on Tuesday (Mon is a holiday here), but if 
> anyone wants to get to it sooner, what I plan to do is:
>
> - Add a new `Extension` diagnostic about allowing you to define a type in 
> `__builtin_offsetof`
> - Replace the error in C with the extension warning (C++ will continue to err)
> - Add documentation to the extensions manual for the builtin
> - Update the release note and tests accordingly

It might be worth to revert the patch in the meantime to reduce the impact on 
adopters that are using current `main` for testing. Among other things, it also 
breaks building the gcc workload or SPEC2017


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-15 Thread Ron Lieberman via Phabricator via cfe-commits

ronlieb added a comment.

our nekbone app is failing from this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

I'll take care of fixing this up on Tuesday (Mon is a holiday here), but if 
anyone wants to get to it sooner, what I plan to do is:

- Add a new `Extension` diagnostic about allowing you to define a type in 
`__builtin_offsetof`
- Replace the error in C with the extension warning (C++ will continue to err)
- Add documentation to the extensions manual for the builtin
- Update the release note and tests accordingly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-14 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added a comment.

+1 on this being disruptive. Also, not only GCC, but also MSVC seem to handle 
the existing constructs in the wild. I ran into this at 
https://github.com/FFmpeg/FFmpeg/blob/n5.1.2/libavutil/video_enc_params.c#L30-L34
 (where it probably would be trivial to fix, but leaving current releases 
broken with newer clang) and at 
https://github.com/wine-mirror/wine/blob/wine-8.0-rc4/include/winnt.h#L399-L405 
(where it works with clang in the usual gnuc mode, but falls back on the UB 
construct with clang in msvc mode).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4052293 , @efriedma wrote:

> Given the extension appears to have real-world usage, I don't see any reason 
> to break that usage.  All known implementations parse this the way the user 
> expects, there isn't any chance the user meant to write something else, and 
> there isn't any chance it will mean something else in a future version of the 
> standard.

I've reached out privately to some folks in GCC to find out whether they plan 
to define this behavior in their builtin or not. One response I've gotten so 
far is that they expect a type to be allowed to be defined in C but not in C++ 
(so `offsetof` has the usual expected behavior in each language). Because of 
the existing code and because of GCC, I'm leaning pretty strongly towards 
defining the behavior for C and issuing an extension warning. If anyone has a 
strong opinion to the contrary, please speak up!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added a comment.

Given the extension appears to have real-world usage, I don't see any reason to 
break that usage.  All known implementations parse this the way the user 
expects, there isn't any chance the user meant to write something else, and 
there isn't any chance it will mean something else in a future version of the 
standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Nikolas Klauser via Phabricator via cfe-commits

philnik added a comment.

In D133574#4051899 , @aaron.ballman 
wrote:

> In D133574#4051782 , @bkramer wrote:
>
>> GitHub finds around 1.9k instances of this pattern to compute `alignof`. 
>> There's a lot of duplicates and `#ifdefs` so the real number is going to be 
>> smaller, but it seems to be quite common. The annoying part is that there is 
>> no `_Alignof` in C99 so for many of those projects there is no easy fix.
>>
>> Can this be demoted to a warning?
>>
>> https://github.com/search?type=code=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac
>
> Adding clang-vendors/others with opinions on diagnostic severity because this 
> could be disruptive.
>
> 1. Yes, we can.
> 2. Blarg. I have sympathy because the standard was unclear on this point for 
> so long. I don't have as much sympathy for "there's no easy fix" -- the code 
> is not portable as-is (it's UB), and Clang supports `_Alignof` in C99 and 
> earlier, so for Clang there is an easy fix.
> 3. Instead, should we downgrade to a warning-defaults-to-error instead of 
> just a warning? That gives these folks a path forward while still making it 
> clear to users "don't do this." But if we think we won't reasonably be able 
> to turn this into an error, I think a warning is better.
> 4. Alternatively, should we document that we support this as an extension and 
> only give an extension warning for it?
>
> Regardless, we need to either make a decision shortly or we should revert 
> these changes until after the Clang 16 branch and re-land after that to give 
> ourselves more time to make a decision. If we don't have an obvious "this is 
> the right approach" answer and an implementation by Jan 19 (next Thursday), I 
> think we should revert to give ourselves breathing room.

I'm not a vendor, but another option would be to make it a 
warning-defaults-to-error and indicate that it will be promoted to an error in 
a few (2?) releases. That makes it less painful, but still make it an error 
eventually. I don't know if clang has done anything like this before though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Thomas Lively via Phabricator via cfe-commits

tlively added a comment.

FWIW I had to patch the musl used by Emscripten to work around this: 
https://github.com/emscripten-core/emscripten/pull/18510

The fix was simple and I don't have a strong opinion about the right way 
forward, but this change was slightly disruptive and I imagine other projects 
will have to work around it as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Marking this as needing changes so it doesn't look accepted to the new 
reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman reopened this revision.
aaron.ballman added reviewers: clang-vendors, mgorny, thesamesam.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D133574#4051782 , @bkramer wrote:

> GitHub finds around 1.9k instances of this pattern to compute `alignof`. 
> There's a lot of duplicates and `#ifdefs` so the real number is going to be 
> smaller, but it seems to be quite common. The annoying part is that there is 
> no `_Alignof` in C99 so for many of those projects there is no easy fix.
>
> Can this be demoted to a warning?
>
> https://github.com/search?type=code=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac

Adding clang-vendors/others with opinions on diagnostic severity because this 
could be disruptive.

1. Yes, we can.
2. Blarg. I have sympathy because the standard was unclear on this point for so 
long. I don't have as much sympathy for "there's no easy fix" -- the code is 
not portable as-is (it's UB), and Clang supports `_Alignof` in C99 and earlier, 
so for Clang there is an easy fix.
3. Instead, should we downgrade to a warning-defaults-to-error instead of just 
a warning? That gives these folks a path forward while still making it clear to 
users "don't do this." But if we think we won't reasonably be able to turn this 
into an error, I think a warning is better.
4. Alternatively, should we document that we support this as an extension and 
only give an extension warning for it?

Regardless, we need to either make a decision shortly or we should revert these 
changes until after the Clang 16 branch and re-land after that to give 
ourselves more time to make a decision. If we don't have an obvious "this is 
the right approach" answer and an implementation by Jan 19 (next Thursday), I 
think we should revert to give ourselves breathing room.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-13 Thread Benjamin Kramer via Phabricator via cfe-commits

bkramer added a comment.

GitHub finds around 1.9k instances of this pattern to compute `alignof`. 
There's a lot of duplicates and `#ifdefs` so the real number is going to be 
smaller, but it seems to be quite common. The annoying part is that there is no 
`_Alignof` in C99 so for many of those projects there is no easy fix.

Can this be demoted to a warning?

https://github.com/search?type=code=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-12 Thread Yingchi Long via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe327b52766ed: [C2x] reject type definitions in offsetof 
(authored by inclyc).

Changed prior to commit:
  https://reviews.llvm.org/D133574?vs=482719=488877#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/C/drs/dr4xx.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/drs/dr4xx.c
===
--- clang/test/C/drs/dr4xx.c
+++ clang/test/C/drs/dr4xx.c
@@ -352,11 +352,10 @@
  */
 
   /* The DR asked a question about whether defining a new type within offsetof
-   * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
-   * has always supported defining a type in this location, and GCC also
-   * supports it.
+   * is allowed. C2x N2350 made this explicitly undefined behavior, but GCC
+   * supports it, Clang diagnoses this a UB and rejects it.
*/
-   (void)__builtin_offsetof(struct S { int a; }, a);
+   (void)__builtin_offsetof(struct S { int a; }, a); /* expected-error{{'struct S' cannot be defined in '__builtin_offsetof'}} */
 }
 
 /* WG14 DR499: yes
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,67 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+int anonymous_struct() {
+  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}
+  { 
+int a;
+int b;
+  }, a);
+}
+
+int struct_in_second_param() {
+  struct A {
+int a, b;
+int x[20];
+  };
+  return __builtin_offsetof(struct A, x[sizeof(struct B{int a;})]); // no-error
+}
+
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'struct A' cannot be defined in 'offsetof'}}
+   // expected-error@-1{{'struct B' cannot be defined in 'offsetof'}}
+  { 
+int a;
+struct B // verifier seems to think the error is emitted by the macro
+ // In fact the location of the error is "B" on the line above
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for

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

2023-01-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#4047677 , @inclyc wrote:

> ping :)

Oh! I missed your pings because this was already marked as accepted, I'm sorry 
for the delay in answering your question.

In D133574#3994050 , @inclyc wrote:

> Hi @aaron.ballman! seems we are failing tests on recent change 
> 2fc5a3410087c209567c7e4a2c457b6896c127a3 
> 
>
>   /* The DR asked a question about whether defining a new type within offsetof
>* is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
>* has always supported defining a type in this location, and GCC also
>* supports it.
>*/
>(void)__builtin_offsetof(struct S { int a; }, a);
>
> Do you think this patch should still be merged into mainline? Or do we turn 
> this error into a warning?

We don't currently document `__builtin_offsetof` and GCC's documentation 
(https://gcc.gnu.org/onlinedocs/gcc/Offsetof.html) does not explicitly claim 
they support this behavior, so I'm fine with diagnosing this UB when we 
previously didn't. If we find this breaks significant code in practice, maybe 
we'll come to another decision, but for now I'd say you should update that test 
to expect the error (and update the comment as well).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-12 Thread Yingchi Long via Phabricator via cfe-commits

inclyc added a comment.

ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-01-01 Thread Yingchi Long via Phabricator via cfe-commits

inclyc added a comment.

ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-12-13 Thread Yingchi Long via Phabricator via cfe-commits

inclyc added a comment.

Hi @aaron.ballman seems we are failing on recent change 
2fc5a3410087c209567c7e4a2c457b6896c127a3 


  /* The DR asked a question about whether defining a new type within offsetof
   * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
   * has always supported defining a type in this location, and GCC also
   * supports it.
   */
   (void)__builtin_offsetof(struct S { int a; }, a);

Do you think this patch should still be merged into mainline? Or do we turn 
this error into a warning?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-12-13 Thread Yingchi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 482719.
inclyc added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,67 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+int anonymous_struct() {
+  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}
+  { 
+int a;
+int b;
+  }, a);
+}
+
+int struct_in_second_param() {
+  struct A {
+int a, b;
+int x[20];
+  };
+  return __builtin_offsetof(struct A, x[sizeof(struct B{int a;})]); // no-error
+}
+
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'struct A' cannot be defined in 'offsetof'}}
+   // expected-error@-1{{'struct B' cannot be defined in 'offsetof'}}
+  { 
+int a;
+struct B // verifier seems to think the error is emitted by the macro
+ // In fact the location of the error is "B" on the line above
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for traditional offsetof as a function-like macro
+int * macro_func(void) {
+  return offsetof(struct A // no-warning
+  { 
+int a;
+int b;
+  }, a);
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10181,13 +10181,12 @@
 
   bool Owned = false;
   bool IsDependent = false;
-  Decl *TagD = ActOnTag(S, TagSpec, Sema::TUK_Reference,
-KWLoc, SS, Name, NameLoc, Attr, AS_none,
-/*ModulePrivateLoc=*/SourceLocation(),
-MultiTemplateParamsArg(), Owned, IsDependent,
-SourceLocation(), false, TypeResult(),
-/*IsTypeSpecifier*/false,
-/*IsTemplateParamOrArg*/false);
+  Decl *TagD = ActOnTag(
+  S, TagSpec, Sema::TUK_Reference, KWLoc, SS, Name, NameLoc, Attr, AS_none,
+  /*ModulePrivateLoc=*/SourceLocation(), MultiTemplateParamsArg(), Owned,
+  IsDependent, SourceLocation(), false, TypeResult(),
+

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

2022-09-27 Thread Nathan Chancellor via Phabricator via cfe-commits

nathanchance added a comment.

Diff 463063 looks good against the Linux kernel with 
https://lore.kernel.org/20220925153151.2467884-1...@inclyc.cn/ applied. I see 
no additional errors/warnings and the 
`drivers/media/platform/nvidia/tegra-vde/v4l2.c` did disappear. Once that patch 
has been fully chased and accepted, this change should be able to be merged 
without any problems. Thank you again for taking a look at the kernel ahead of 
time, it is very much appreciated!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#3816688 , @inclyc wrote:

> This revision fixes:
>
> **anonymous struct**
>
>> You should be able to pass in the TagDecl directly because the diagnostics
>> engine knows how to print a NamedDecl.
>
> I've switch back to using `Context.getTagDeclType(New)` because this can print
> anonymous pretty.
>
>   clang/test/C/C2x/n2350.c:23:29: error: 'struct (unnamed at 
> clang/test/C/C2x/n2350.c:23:29)' cannot be defined in '__builtin_offsetof'
> return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed 
> at {{.*}})' cannot be defined in '__builtin_offsetof'}}

Thanks! I was very surprised we didn't print the anonymous decl properly from 
the decl itself, so it looks like we have the same bug in other places as well: 
https://godbolt.org/z/16vP3voTW

> **definitions within the second parameter**
>
> I've add a new test case for this. This bug is caused by the RAIIObject having
> incorrect scope.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 463063.
inclyc added a comment.

This revision fixes:

**anonymous struct**

> You should be able to pass in the TagDecl directly because the diagnostics
> engine knows how to print a NamedDecl.

I've switch back to using `Context.getTagDeclType(New)` because this can print
anonymous pretty.

  clang/test/C/C2x/n2350.c:23:29: error: 'struct (unnamed at 
clang/test/C/C2x/n2350.c:23:29)' cannot be defined in '__builtin_offsetof'
return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at 
{{.*}})' cannot be defined in '__builtin_offsetof'}}

**definitions within the second parameter**

I've add a new test case for this. This bug is caused by the RAIIObject having
incorrect scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,67 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+int anonymous_struct() {
+  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}
+  { 
+int a;
+int b;
+  }, a);
+}
+
+int struct_in_second_param() {
+  struct A {
+int a, b;
+int x[20];
+  };
+  return __builtin_offsetof(struct A, x[sizeof(struct B{int a;})]); // no-error
+}
+
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'struct A' cannot be defined in 'offsetof'}}
+   // expected-error@-1{{'struct B' cannot be defined in 'offsetof'}}
+  { 
+int a;
+struct B // verifier seems to think the error is emitted by the macro
+ // In fact the location of the error is "B" on the line above
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for traditional offsetof as a function-like macro
+int * macro_func(void) {
+  return offsetof(struct A // no-warning
+  { 
+int a;
+int b;
+  }, a);
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10053,13 +10053,12 @@
 
   bool Owned = false;
   bool IsDependent = false;
-  Decl *TagD =

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

2022-09-26 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#3816066 , @nathanchance 
wrote:

> In D133574#3815951 , @aaron.ballman 
> wrote:
>
>> LGTM, thank you! Please don't land until you have some indication from the 
>> kernel folks that this won't be super disruptive for them. CC 
>> @nickdesaulniers and @nathanchance for awareness.
>
> Thank you a lot for the heads up! I see an in-flight change around this from 
> YingChi on the mailing list:
>
> https://lore.kernel.org/20220925153151.2467884-1...@inclyc.cn/
>
> It would be good to have that change accepted into a maintainer's tree and on 
> its way to mainline (Linus's tree) and by extension, the stable releases 
> before merging this into main, as we and several other groups are actively 
> testing all Linux branches with LLVM main to catch other regressions and this 
> error would require patching of our CI, which we can definitely do but prefer 
> to avoid because of the maintenance overhead (we try to carry zero patches at 
> all times).

I don't think there's a rush to land this immediately, so we can definitely 
wait for you to give the all-clear.

> I am still running my set of builds against the kernel with this LLVM change 
> and the aforementioned Linux change but I do see another error:
>
>   drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be 
> defined in 'offsetof'
>   ctx = kzalloc(offsetof(struct tegra_ctx, 
> ctrls[ARRAY_SIZE(ctrl_cfgs)]),
>  ^
>   include/linux/kernel.h:55:59: note: expanded from macro 'ARRAY_SIZE'
>   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
> __must_be_array(arr))
> ^
>   include/linux/compiler.h:240:28: note: expanded from macro '__must_be_array'
>   #define __must_be_array(a)  BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>   ^
>   include/linux/build_bug.h:16:44: note: expanded from macro 
> 'BUILD_BUG_ON_ZERO'
>   #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>  ^
>   1 error generated.
>
> I will report back with any additional problems found.

Oh wow, that's a fun one for a few reasons. 1) Oops, we print `''` instead of 
mention that it's an anonymous struct, 2) that structure is actually declared 
in the scope surrounding the `sizeof` so it is technically being defined within 
`offsetof`, but 3) The standard requirement is "If the specified type defines a 
new type or if the specified member is a bit-field, the behavior is undefined." 
where "type" is the first parameter of the `offsetof` macro and this example 
does not define a new type there.

So I *think* this situation is actually a false positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread Nick Desaulniers via Phabricator via cfe-commits

nickdesaulniers added a comment.

In D133574#3816066 , @nathanchance 
wrote:

> In D133574#3815951 , @aaron.ballman 
> wrote:
>
>> LGTM, thank you! Please don't land until you have some indication from the 
>> kernel folks that this won't be super disruptive for them. CC 
>> @nickdesaulniers and @nathanchance for awareness.
>
> Thank you a lot for the heads up! I see an in-flight change around this from 
> YingChi on the mailing list:
>
> https://lore.kernel.org/20220925153151.2467884-1...@inclyc.cn/
>
> It would be good to have that change accepted into a maintainer's tree and on 
> its way to mainline (Linus's tree) and by extension, the stable releases 
> before merging this into main, as we and several other groups are actively 
> testing all Linux branches with LLVM main to catch other regressions and this 
> error would require patching of our CI, which we can definitely do but prefer 
> to avoid because of the maintenance overhead (we try to carry zero patches at 
> all times).
>
> I am still running my set of builds against the kernel with this LLVM change 
> and the aforementioned Linux change but I do see another error:
>
>   drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be 
> defined in 'offsetof'

^ perhaps we can say something about an anonymous struct rather than print an 
empty name?

Thanks for the consideration and the kernel patch. Patching the kernel and 
clang is exceptional, going "above and beyond."  Unless we are in a rush to 
land this, having a little more time for feedback from the kernel thread linked 
above I think would be good.

In particular, I was surprised by the distinction between `__alignof__` and 
`_Alignof` semantically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread Nathan Chancellor via Phabricator via cfe-commits

nathanchance added a comment.

In D133574#3815951 , @aaron.ballman 
wrote:

> LGTM, thank you! Please don't land until you have some indication from the 
> kernel folks that this won't be super disruptive for them. CC 
> @nickdesaulniers and @nathanchance for awareness.

Thank you a lot for the heads up! I see an in-flight change around this from 
YingChi on the mailing list:

https://lore.kernel.org/20220925153151.2467884-1...@inclyc.cn/

It would be good to have that change accepted into a maintainer's tree and on 
its way to mainline (Linus's tree) and by extension, the stable releases before 
merging this into main, as we and several other groups are actively testing all 
Linux branches with LLVM main to catch other regressions and this error would 
require patching of our CI, which we can definitely do but prefer to avoid 
because of the maintenance overhead (we try to carry zero patches at all times).

I am still running my set of builds against the kernel with this LLVM change 
and the aforementioned Linux change but I do see another error:

  drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be 
defined in 'offsetof'
  ctx = kzalloc(offsetof(struct tegra_ctx, 
ctrls[ARRAY_SIZE(ctrl_cfgs)]),
 ^
  include/linux/kernel.h:55:59: note: expanded from macro 'ARRAY_SIZE'
  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
__must_be_array(arr))
^
  include/linux/compiler.h:240:28: note: expanded from macro '__must_be_array'
  #define __must_be_array(a)  BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
  ^
  include/linux/build_bug.h:16:44: note: expanded from macro 'BUILD_BUG_ON_ZERO'
  #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
 ^
  1 error generated.

I will report back with any additional problems found.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman accepted this revision.
aaron.ballman added a subscriber: nathanchance.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you! Please don't land until you have some indication from the 
kernel folks that this won't be super disruptive for them. CC @nickdesaulniers 
and @nathanchance for awareness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 462942.
inclyc added a comment.

Fix comment nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'A' cannot be defined in 'offsetof'}}
+   // expected-error@-1{{'B' cannot be defined in 'offsetof'}}
+  { 
+int a;
+struct B // verifier seems to think the error is emitted by the macro
+ // In fact the location of the error is "B" on the line above
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for traditional offsetof as a function-like macro
+int * macro_func(void) {
+  return offsetof(struct A // no-warning
+  { 
+int a;
+int b;
+  }, a);
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10053,13 +10053,12 @@
 
   bool Owned = false;
   bool IsDependent = false;
-  Decl *TagD = ActOnTag(S, TagSpec, Sema::TUK_Reference,
-KWLoc, SS, Name, NameLoc, Attr, AS_none,
-/*ModulePrivateLoc=*/SourceLocation(),
-MultiTemplateParamsArg(), Owned, IsDependent,
-SourceLocation(), false, TypeResult(),
-/*IsTypeSpecifier*/false,
-/*IsTemplateParamOrArg*/false);
+  Decl *TagD = ActOnTag(
+  S, TagSpec, Sema::TUK_Reference, KWLoc, SS, Name, NameLoc, Attr, AS_none,
+  /*ModulePrivateLoc=*/SourceLocation(), MultiTemplateParamsArg(), Owned,
+  IsDependent, SourceLocation(), false, TypeResult(),
+  /*IsTypeSpecifier*/ false,
+  /*IsTemplateParamOrArg=*/false, /*OOK=*/OOK_Outside);
   assert(!IsDependent && "explicit instantiation of dependent name not yet handled");
 
   if (!TagD)
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -16922,15 +16922,15 @@

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 462940.
inclyc added a comment.

Address comments from @aaron.ballman

Move `OffsetOfKind` to `Sema` and pass it to `Sema:ActOnTag` directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'A' cannot be defined in 'offsetof'}}
+   // expected-error@-1{{'B' cannot be defined in 'offsetof'}}
+  { 
+int a;
+struct B // verifier seems to think the error is emitted by the macro
+ // In fact the location of the error is "B" on the line above
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for traditional offsetof as a function-like macro
+int * macro_func(void) {
+  return offsetof(struct A // no-warning
+  { 
+int a;
+int b;
+  }, a);
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -10053,13 +10053,12 @@
 
   bool Owned = false;
   bool IsDependent = false;
-  Decl *TagD = ActOnTag(S, TagSpec, Sema::TUK_Reference,
-KWLoc, SS, Name, NameLoc, Attr, AS_none,
-/*ModulePrivateLoc=*/SourceLocation(),
-MultiTemplateParamsArg(), Owned, IsDependent,
-SourceLocation(), false, TypeResult(),
-/*IsTypeSpecifier*/false,
-/*IsTemplateParamOrArg*/false);
+  Decl *TagD = ActOnTag(
+  S, TagSpec, Sema::TUK_Reference, KWLoc, SS, Name, NameLoc, Attr, AS_none,
+  /*ModulePrivateLoc=*/SourceLocation(), MultiTemplateParamsArg(), Owned,
+  IsDependent, SourceLocation(), false, TypeResult(),
+  /*IsTypeSpecifier*/ false,
+  /*IsOffsetOfInMacro=*/false, /*OOK=*/OOK_Outside);
   assert(!IsDependent && "explicit instantiation of dependent name not yet handled");
 
   if (!TagD)
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
---

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

2022-09-26 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3279
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
+ bool IsWithinOffsetOf, bool IsOffsetOfInMacro,
  SkipBodyInfo *SkipBody = nullptr);

inclyc wrote:
> aaron.ballman wrote:
> > Instead of passing two `bool`s, why not pass `Parser::OffsetOfStateKind` 
> > directly?
> `Parser::OffsetOfStateKind` is not visible to class `Sema`?
Sure, but it's a new enum, so we can put it into `Sema` if we want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc added a comment.

related kernel patch https://lkml.org/lkml/2022/9/26/1484


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:3279
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
+ bool IsWithinOffsetOf, bool IsOffsetOfInMacro,
  SkipBodyInfo *SkipBody = nullptr);

aaron.ballman wrote:
> Instead of passing two `bool`s, why not pass `Parser::OffsetOfStateKind` 
> directly?
`Parser::OffsetOfStateKind` is not visible to class `Sema`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a subscriber: nickdesaulniers.
aaron.ballman added a comment.

In D133574#3815105 , @inclyc wrote:

> Hi @aaron.ballman, I've noticed in the linux kernel, type alignment was 
> implemented by a tricky way using offsetof.
>
>   #define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
>
> Does this always has the same semantic with C11 `_Alignof`? If this is not 
> true, I think unfortunately we may emit a switchable warning instead of an 
> error to preserve semantics here.

Technically there's no requirement that they return the same value (the 
structure could insert arbitrary padding, including no padding), so it's 
theoretically possible they return different values. But I can't think of a 
situation in which you'd get a different answer from `TYPE_ALIGN` as you would 
get from `_Alignof`.

@nickdesaulniers any thoughts here since this can impact the Linux kernel?

Comment at: clang/include/clang/Parse/Parser.h:252-257
+// Not parsing a type within __builtin_offsetof
+Outside,
+// Parsing a type within __builtin_offsetof
+Builtin,
+// Parsing a type within macro "offsetof", defined in __buitin_offsetof
+// To improve our diagnostic message

Comment at: clang/include/clang/Sema/Sema.h:3279
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
+ bool IsWithinOffsetOf, bool IsOffsetOfInMacro,
  SkipBodyInfo *SkipBody = nullptr);

Instead of passing two `bool`s, why not pass `Parser::OffsetOfStateKind` 
directly?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-26 Thread YingChi Long via Phabricator via cfe-commits

inclyc added a comment.

Hi @aaron.ballman, I've noticed in the linux kernel, type alignment was 
implemented by a tricky way using offsetof.

  #define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)

Does this always has the same semantic with C11 `_Alignof`? If this is not 
true, I think unfortunately we may emit a switchable warning instead of an 
error to preserve semantics here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-24 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 462680.
inclyc added a comment.

Address comments.

Clang will now consider __builtin_offsetof #defined from "offsetof" to improve
diagnostic message.

For example:

  #define offsetof(t, d) __builtin_offsetof(t, d)
  
  int main() {
return offsetof(struct S { int a; }, a);
  }



  local/offsetof.c:4:26: error: 'S' cannot be defined in 'offsetof'
return offsetof(struct S { int a; }, a);
   ^
  1 error generated.

Emm, the "expected-error" of struct B within a macro seems have to be annotated
at the same line as"struct A".

  int macro(void) {
return offsetof(struct A // expected-error{{'A' cannot be defined in 
'offsetof'}}
 // expected-error@-1{{'B' cannot be defined in 
'offsetof'}} < Have to write this here, but I believe the line number 
is correct
{ 
  int a;
  struct B // FIXME: verifier seems to think the error is emitted by the 
macro
   // In fact the location of the error is "B" on the line above
  {
int c;
int d;
  } x;
}, a);
  }



  clang/test/C/C2x/n2350.c:11:36: error: 'A' cannot be defined in 
'__builtin_offsetof'
return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined 
in '__builtin_offsetof'}} 
 ^
  clang/test/C/C2x/n2350.c:14:12: error: 'B' cannot be defined in 
'__builtin_offsetof'
  struct B // expected-error{{'B' cannot be defined in 
'__builtin_offsetof'}} 
 ^
  clang/test/C/C2x/n2350.c:26:26: error: 'A' cannot be defined in 'offsetof'
return offsetof(struct A // expected-error{{'A' cannot be defined in 
'offsetof'}}
   ^
  clang/test/C/C2x/n2350.c:30:12: error: 'B' cannot be defined in 'offsetof' 
<-- note that this line number is "30" (not "26")
  struct B // FIXME: verifier seems to think the error is emitted by the 
macro
 ^
  4 errors generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/C/C2x/n2350.c
  clang/test/Parser/declarators.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,20 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // FIXME: error diagnostic message for nested definitions 
+ // https://reviews.llvm.org/D133574 
+ // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/test/C/C2x/n2350.c
===
--- /dev/null
+++ clang/test/C/C2x/n2350.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'B' cannot be defined in '__builtin_offsetof'}} 
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'A' cannot be defined in 'offsetof'}}
+

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

2022-09-23 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added a comment.

LGTM but I will Aaron give the final approval.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 

inclyc wrote:
> aaron.ballman wrote:
> > inclyc wrote:
> > > aaron.ballman wrote:
> > > > We might want this change, we might not -- can you test the diagnostic 
> > > > behavior when using `#include `? Does it print 
> > > > `__builtin_offsetof` in the following example?
> > > > ```
> > > > #include 
> > > > 
> > > > int main() {
> > > >   return offsetof(struct S { int a; }, a);
> > > > }
> > > > ```
> > > > when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`
> > > > 
> > > > If it prints the builtin name, I think we'll want to look at the 
> > > > builtin token to see if it was expanded from a macro named `offsetof` 
> > > > to improve the diagnostic quality.
> > > ```
> > > local/offsetofcc.c:4:26: error: 'struct S' cannot be defined in 
> > > '__builtin_offsetof'
> > >   return offsetof(struct S { int a; }, a);
> > >  ^
> > > 1 error generated.
> > > ```
> > > > If it prints the builtin name, I think we'll want to look at the 
> > > > builtin token to see if it was expanded from a macro named offsetof to 
> > > > improve the diagnostic quality.
> > > 
> > > OK
> > We have similar code for this here: 
> > 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaChecking.cpp#L13438
> > We have similar code for this here:
> 
> Wow! Thank you so much for this. I'm searching for how to do this in a LOT of 
> doxygen generated pages (
No problem! I realized "this could be tricky, surely we've done this before" 
and went looking. :-D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-23 Thread YingChi Long via Phabricator via cfe-commits

inclyc added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 

aaron.ballman wrote:
> inclyc wrote:
> > aaron.ballman wrote:
> > > We might want this change, we might not -- can you test the diagnostic 
> > > behavior when using `#include `? Does it print 
> > > `__builtin_offsetof` in the following example?
> > > ```
> > > #include 
> > > 
> > > int main() {
> > >   return offsetof(struct S { int a; }, a);
> > > }
> > > ```
> > > when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`
> > > 
> > > If it prints the builtin name, I think we'll want to look at the builtin 
> > > token to see if it was expanded from a macro named `offsetof` to improve 
> > > the diagnostic quality.
> > ```
> > local/offsetofcc.c:4:26: error: 'struct S' cannot be defined in 
> > '__builtin_offsetof'
> >   return offsetof(struct S { int a; }, a);
> >  ^
> > 1 error generated.
> > ```
> > > If it prints the builtin name, I think we'll want to look at the builtin 
> > > token to see if it was expanded from a macro named offsetof to improve 
> > > the diagnostic quality.
> > 
> > OK
> We have similar code for this here: 
> 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaChecking.cpp#L13438
> We have similar code for this here:

Wow! Thank you so much for this. I'm searching for how to do this in a LOT of 
doxygen generated pages (


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#3811496 , @inclyc wrote:

> Emm, is it necessary to add a `LangOpts` check so that this change only 
> applies to c2x? If clang was invoked without `-std=c2x`, should we just 
> accept `offsetof` with definitions?

No, I think we should treat this as a defect report and have the behavior be 
the same in all versions of C.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 

inclyc wrote:
> aaron.ballman wrote:
> > We might want this change, we might not -- can you test the diagnostic 
> > behavior when using `#include `? Does it print 
> > `__builtin_offsetof` in the following example?
> > ```
> > #include 
> > 
> > int main() {
> >   return offsetof(struct S { int a; }, a);
> > }
> > ```
> > when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`
> > 
> > If it prints the builtin name, I think we'll want to look at the builtin 
> > token to see if it was expanded from a macro named `offsetof` to improve 
> > the diagnostic quality.
> ```
> local/offsetofcc.c:4:26: error: 'struct S' cannot be defined in 
> '__builtin_offsetof'
>   return offsetof(struct S { int a; }, a);
>  ^
> 1 error generated.
> ```
> > If it prints the builtin name, I think we'll want to look at the builtin 
> > token to see if it was expanded from a macro named offsetof to improve the 
> > diagnostic quality.
> 
> OK
We have similar code for this here: 

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaChecking.cpp#L13438


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-23 Thread YingChi Long via Phabricator via cfe-commits

inclyc added a comment.

Emm, is it necessary to add a `LangOpts` check so that this change only applies 
to c2x? If clang was invoked without `-std=c2x`, should we just accept 
`offsetof` with definitions?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 

aaron.ballman wrote:
> We might want this change, we might not -- can you test the diagnostic 
> behavior when using `#include `? Does it print `__builtin_offsetof` 
> in the following example?
> ```
> #include 
> 
> int main() {
>   return offsetof(struct S { int a; }, a);
> }
> ```
> when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`
> 
> If it prints the builtin name, I think we'll want to look at the builtin 
> token to see if it was expanded from a macro named `offsetof` to improve the 
> diagnostic quality.
```
local/offsetofcc.c:4:26: error: 'struct S' cannot be defined in 
'__builtin_offsetof'
  return offsetof(struct S { int a; }, a);
 ^
1 error generated.
```
> If it prints the builtin name, I think we'll want to look at the builtin 
> token to see if it was expanded from a macro named offsetof to improve the 
> diagnostic quality.

OK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:279-280
   ``%hd``.
+- Reject type definitions in the ``type`` argument of ``__builtin_offsetof`` 
+  according to `WG14 N2350 
`_.
 

You should move this one down to the C2x Feature Support section instead, since 
it's a C2x paper.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 

We might want this change, we might not -- can you test the diagnostic behavior 
when using `#include `? Does it print `__builtin_offsetof` in the 
following example?
```
#include 

int main() {
  return offsetof(struct S { int a; }, a);
}
```
when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`

If it prints the builtin name, I think we'll want to look at the builtin token 
to see if it was expanded from a macro named `offsetof` to improve the 
diagnostic quality.



Comment at: clang/lib/Sema/SemaDecl.cpp:17034
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;

You should be able to pass in the `TagDecl` directly because the diagnostics 
engine knows how to print a `NamedDecl`.



Comment at: clang/test/Sema/offsetof.c:73-75
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {

Can you move this into a test named `clang/test/C/C2x/n2350.c` which looks like 
the other tests in that directory? That's the testing directory we're putting 
some basic validation tests for C papers we've implemented so that we can 
better track what features we claim to support.



Comment at: clang/test/SemaCXX/offsetof.cpp:93
+int a;
+struct B
+{

I'd like a FIXME comment here about wanting to diagnose this someday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-21 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 461822.
inclyc added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,18 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16256,7 +16256,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17029,10 +17029,16 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinOffsetOf && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
-  if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
-  TUK == TUK_Definition) {
+  if (!Invalid && getLangOpts().CPlusPlus &&
+  (IsTypeSpecifier || IsTemplateParamOrArg) && TUK == TUK_Definition) {
 Diag(New->getLocation(), diag::err_type_defined_in_type_specifier)
   << Context.getTagDeclType(New);
 Invalid = true;
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,6 +2579,7 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
+InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
 TypeResult Ty = ParseTypeName();
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC == DeclSpecContext::DSC_type_specifier,
 DSC == DeclSpecContext::DSC_template_param ||
 DSC == DeclSpecContext::DSC_template_type_arg,
-);
+, InBuiltInOffsetOfBase);
 
 // If ActOnTag said the type was dependent, try again with the
 // less common call.
Index: clang/include/clang/Sema/Sema.h
===
---

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

2022-09-19 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 461463.
inclyc added a comment.

Switch back to RAIIObject.

Currently clang could not generate diagnostic messages for nested definitions
in C++. I believe using RAIIObject here is logically correct, but in C++ mode,
`ActOnTag` returns `nullptr`, and the comment says

> We can't recover well from the cases where we make the type anonymous



  /* SemaDecl.cpp Sema::ActOnTag Ln 17189 */
  // In C++, don't return an invalid declaration. We can't recover well from
  // the cases where we make the type anonymous.
  if (Invalid && getLangOpts().CPlusPlus) {
if (New->isBeingDefined())
  if (auto RD = dyn_cast(New))
RD->completeDefinition();
return nullptr;
  } else if (SkipBody && SkipBody->ShouldSkip) {
return SkipBody->Previous;
  } else {
return New;
  }

I believe the state "ParsingBuiltinOffsetof" is correctly passed into
`ParseCXXMemberSpecification` (parsing nested definition), and if clang
recovers invalid declaration in the furture, nested definitions will be caught.

  if (TUK == Sema::TUK_Definition) {
assert(Tok.is(tok::l_brace) ||
   (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
   isClassCompatibleKeyword());
if (SkipBody.ShouldSkip)
  SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
 TagOrTempResult.get());
else if (getLangOpts().CPlusPlus)
  ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
  TagOrTempResult.get());  // <  nullptr, 
nested declaration skipped
else {
  Decl *D =
  SkipBody.CheckSameAsPrevious ? SkipBody.New : TagOrTempResult.get();
  // Parse the definition body.
  ParseStructUnionBody(StartLoc, TagType, cast(D));
  if (SkipBody.CheckSameAsPrevious &&
  !Actions.ActOnDuplicateDefinition(TagOrTempResult.get(), SkipBody)) {
DS.SetTypeSpecError();
return;
  }
}
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -83,3 +83,18 @@
   expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B
+{
+  int c;
+  int d;
+};
+B x;
+  }, a);
+}
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo

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

2022-09-16 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman 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
+{

inclyc wrote:
> 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 
> > > ```
> > > : In function 'int main()':
> > > :3:3: error: types may not be defined within '__builtin_offsetof'
> > > 3 |   {
> > >   |   ^
> > > :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?
> 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

Yeah, that is interesting!

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

I'm getting more comfortable with that approach. Please be sure to add C++ 
tests to make sure we diagnose in C and C++ the same way in terms of nested 
structures. Thank you for exploring the approaches with me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-15 Thread 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 
> > ```
> > : In function 'int main()':
> > :3:3: error: types may not be defined within '__builtin_offsetof'
> > 3 |   {
> >   |   ^
> > :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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-15 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman 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
+{

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 
> ```
> : In function 'int main()':
> :3:3: error: types may not be defined within '__builtin_offsetof'
> 3 |   {
>   |   ^
> :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++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-14 Thread 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
+{

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 
```
: In function 'int main()':
:3:3: error: types may not be defined within '__builtin_offsetof'
3 |   {
  |   ^
:6:5: error: types may not be defined within '__builtin_offsetof'
6 | {
  | ^
Compiler returned: 1
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-14 Thread 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:
> > 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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2311
 case DeclSpecContext::DSC_association:
+case DeclSpecContext::DSC_offsetof:
   return true;

inclyc wrote:
> aaron.ballman wrote:
> > Is this correct? I don't think we can deduce the type from `offsetof` 
> > through CTAD, can we?
> > 
> > https://godbolt.org/z/Kab6ahYe7
> > 
> > (This might be a good test case to add assuming we don't already have that 
> > coverage.)
> Emm, these checks just return as the same as "type_specifier". Because that's 
> what we passed into "ParsingTypename" before.
Hmmm, that's certainly reasonable. Can you add the test case and make sure the 
behavior doesn't change?



Comment at: clang/test/Sema/offsetof.c:79
+int a;
+struct B // no-error, struct B is not defined within __builtin_offsetof 
directly
+{

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-14 Thread YingChi Long via Phabricator via cfe-commits

inclyc added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2311
 case DeclSpecContext::DSC_association:
+case DeclSpecContext::DSC_offsetof:
   return true;

aaron.ballman wrote:
> Is this correct? I don't think we can deduce the type from `offsetof` through 
> CTAD, can we?
> 
> https://godbolt.org/z/Kab6ahYe7
> 
> (This might be a good test case to add assuming we don't already have that 
> coverage.)
Emm, these checks just return as the same as "type_specifier". Because that's 
what we passed into "ParsingTypename" before.



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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2311
 case DeclSpecContext::DSC_association:
+case DeclSpecContext::DSC_offsetof:
   return true;

Is this correct? I don't think we can deduce the type from `offsetof` through 
CTAD, can we?

https://godbolt.org/z/Kab6ahYe7

(This might be a good test case to add assuming we don't already have that 
coverage.)



Comment at: clang/include/clang/Sema/DeclSpec.h:2069
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   return true;

I don't think we can omit the identifier in this context -- this function is 
used to see whether you can do something like:
```
template 
void func(int) {
  try {
  } catch (int) {
  }
}
```
and `offsetof` seems quite different from that.



Comment at: clang/test/Sema/offsetof.c:79
+int a;
+struct B // no-error, struct B is not defined within __builtin_offsetof 
directly
+{

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-12 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459634.
inclyc added a comment.

git-clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // no-error, struct B is not defined within __builtin_offsetof directly
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3584,6 +3584,7 @@
   [[fallthrough]];
 case DeclaratorContext::TypeName:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   Error = 15; // Generic
   break;
 case DeclaratorContext::File:
@@ -3695,6 +3696,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   DiagID = diag::err_type_defined_in_type_specifier;
   break;
 case DeclaratorContext::Prototype:
@@ -4784,6 +4786,7 @@
 case DeclaratorContext::FunctionalCast:
 case DeclaratorContext::RequiresExpr:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // Don't infer in these contexts.
   break;
 }
@@ -5836,6 +5839,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // FIXME: We may want to allow parameter packs in block-literal contexts
   // in the future.
   S.Diag(D.getEllipsisLoc(),
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16257,7 +16257,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinBuiltinOffsetof) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17030,6 +17030,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinBuiltinOffsetof && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,8 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
-TypeResult Ty = ParseTypeName();
+TypeResult Ty =
+ParseTypeName(/*Range=*/nullptr, DeclaratorContext::OffsetOf);
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC

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

2022-09-12 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459633.
inclyc added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // no-error, struct B is not defined within __builtin_offsetof directly
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3584,6 +3584,7 @@
   [[fallthrough]];
 case DeclaratorContext::TypeName:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   Error = 15; // Generic
   break;
 case DeclaratorContext::File:
@@ -3695,6 +3696,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   DiagID = diag::err_type_defined_in_type_specifier;
   break;
 case DeclaratorContext::Prototype:
@@ -4784,6 +4786,7 @@
 case DeclaratorContext::FunctionalCast:
 case DeclaratorContext::RequiresExpr:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // Don't infer in these contexts.
   break;
 }
@@ -5836,6 +5839,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // FIXME: We may want to allow parameter packs in block-literal contexts
   // in the future.
   S.Diag(D.getEllipsisLoc(),
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16257,7 +16257,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinBuiltinOffsetof) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17030,6 +17030,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinBuiltinOffsetof && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,8 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
-TypeResult Ty = ParseTypeName();
+TypeResult Ty =
+ParseTypeName(/*Range=*/nullptr, DeclaratorContext::OffsetOf);
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC

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

2022-09-12 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2283
 case DeclSpecContext::DSC_top_level:
+case DeclSpecContext::DSC_offsetof:
   return true;

Why `true` for this case? What does this allow that we want? Do we test it?



Comment at: clang/lib/Parse/ParseExpr.cpp:2583
+TypeResult Ty =
+ParseTypeName(/*Range*/ nullptr, DeclaratorContext::OffsetOf);
 if (Ty.isInvalid()) {




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-12 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459458.
inclyc added a comment.

Use declaration context


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // no-error, struct B is not defined within __builtin_offsetof directly
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3584,6 +3584,7 @@
   [[fallthrough]];
 case DeclaratorContext::TypeName:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   Error = 15; // Generic
   break;
 case DeclaratorContext::File:
@@ -3695,6 +3696,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   DiagID = diag::err_type_defined_in_type_specifier;
   break;
 case DeclaratorContext::Prototype:
@@ -4784,6 +4786,7 @@
 case DeclaratorContext::FunctionalCast:
 case DeclaratorContext::RequiresExpr:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // Don't infer in these contexts.
   break;
 }
@@ -5836,6 +5839,7 @@
 case DeclaratorContext::TemplateArg:
 case DeclaratorContext::TemplateTypeArg:
 case DeclaratorContext::Association:
+case DeclaratorContext::OffsetOf:
   // FIXME: We may want to allow parameter packs in block-literal contexts
   // in the future.
   S.Diag(D.getEllipsisLoc(),
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinBuiltinOffsetof) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17027,6 +17027,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinBuiltinOffsetof && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,8 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
-TypeResult Ty = ParseTypeName();
+TypeResult Ty =
+ParseTypeName(/*Range*/ nullptr, DeclaratorContext::OffsetOf);
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@

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

2022-09-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D133574#3781907 , @inclyc wrote:

> Use RAII object to maintain the Parser state
>
>> have you explored making a new DeclSpecContext and modifying 
>> isDefiningTypeSpecifierContext()? I think that would likely be a cleaner 
>> approach.
>
> Emm, I've tried passing a DeclaratorContext into `ParseTypeName()`
>
>   SourceLocation TypeLoc = Tok.getLocation();
>   InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
>   TypeResult Ty = ParseTypeName(nullptr, /*Context=???*/); 
>
> But defining a new DeclaratorContext I have to complete a bunch of `case`
> statements.
>
>   // Parser.h
>   static bool isTypeSpecifier(DeclSpecContext DSC);
>   static AllowDefiningTypeSpec isDefiningTypeSpecifierContext(DeclSpecContext 
> DSC, bool IsCPlusPlus);
>   static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC);
>   /* ... */
>
> And I think it is somehow strange to determine these properties within
> __builtin_offsetof()? I'm not sure if it is really appropriate to define a
> special context for a built-in function. This place should only need to
> forbidden definitions, right?

The thing is: it really is a declaration context; one in which type 
declarations are not allowed. So yes, you do have to fill out a bunch of fully 
covered switches in places, but I still think that's the correct way to model 
this instead of introducing a one-off RAII object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-09 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459248.
inclyc added a comment.

Use RAII object to maintain the Parser state

> have you explored making a new DeclSpecContext and modifying 
> isDefiningTypeSpecifierContext()? I think that would likely be a cleaner 
> approach.

Emm, I've tried passing a DeclaratorContext into `ParseTypeName()`

  SourceLocation TypeLoc = Tok.getLocation();
  InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
  TypeResult Ty = ParseTypeName(nullptr, /*Context=???*/); 

But defining a new DeclaratorContext I have to complete a bunch of `case`
statements.

  // Parser.h
  static bool isTypeSpecifier(DeclSpecContext DSC);
  static AllowDefiningTypeSpec isDefiningTypeSpecifierContext(DeclSpecContext 
DSC, bool IsCPlusPlus);
  static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC);
  /* ... */

And I think it is somehow strange to determine these properties within
__builtin_offsetof()? I'm not sure if it is really appropriate to define a
special context for a built-in function. This place should only need to
forbidden definitions, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Parse/RAIIObjectsForParser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17027,6 +17027,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinOffsetOf && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,6 +2579,7 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
+InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
 TypeResult Ty = ParseTypeName();
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC == DeclSpecContext::DSC_type_specifier,
 DSC == DeclSpecContext::DSC_template_param ||
 DSC == DeclSpecContext::DSC_template_type_arg,
-);
+, InBuiltInOffsetOfBase);
 
 // If ActOnTag said the type was dependent, try again with the
 // less common call.
Index: clang/include/clang/Sema/Sema.h
===
---

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

2022-09-09 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Thank you for working on this! This approach can work (though I would recommend 
using an RAII object akin to `InMessageExpressionRAIIObject`), but have you 
explored making a new `DeclSpecContext` and modifying 
`isDefiningTypeSpecifierContext()`? I think that would likely be a cleaner 
approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-09-09 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459039.
inclyc added a comment.

Use double backquotes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17027,6 +17027,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinOffsetOf && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,9 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
+InBuiltInOffsetOfBase = true;
 TypeResult Ty = ParseTypeName();
+InBuiltInOffsetOfBase = false;
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC == DeclSpecContext::DSC_type_specifier,
 DSC == DeclSpecContext::DSC_template_param ||
 DSC == DeclSpecContext::DSC_template_type_arg,
-);
+, InBuiltInOffsetOfBase);
 
 // If ActOnTag said the type was dependent, try again with the
 // less common call.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -3273,7 +3273,8 @@
  bool , SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody = nullptr);
+ SkipBodyInfo *SkipBody = nullptr,
+ bool IsWithinOffsetOf = false);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
 unsigned TagSpec, SourceLocation TagLoc,
Index: clang/include/clang/Parse/Parser.h
===
--- clang/include/clang/Parse/Parser.h
+++ clang/include/clang/Parse/Parser.h
@@ -246,6 +246,9 @@
   /// function call.
   bool CalledSignatureHelp = false;
 
+  /// Parsing a type within __builtin_offsetof.
+  bool InBuiltInOffsetOfBase = false;
+
   /// The "depth" of the template parameters currently being parsed.

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

2022-09-09 Thread YingChi Long via Phabricator via cfe-commits

inclyc updated this revision to Diff 459037.
inclyc added a comment.

Add release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17027,6 +17027,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinOffsetOf && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,9 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
+InBuiltInOffsetOfBase = true;
 TypeResult Ty = ParseTypeName();
+InBuiltInOffsetOfBase = false;
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC == DeclSpecContext::DSC_type_specifier,
 DSC == DeclSpecContext::DSC_template_param ||
 DSC == DeclSpecContext::DSC_template_type_arg,
-);
+, InBuiltInOffsetOfBase);
 
 // If ActOnTag said the type was dependent, try again with the
 // less common call.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -3273,7 +3273,8 @@
  bool , SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody = nullptr);
+ SkipBodyInfo *SkipBody = nullptr,
+ bool IsWithinOffsetOf = false);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
 unsigned TagSpec, SourceLocation TagLoc,
Index: clang/include/clang/Parse/Parser.h
===
--- clang/include/clang/Parse/Parser.h
+++ clang/include/clang/Parse/Parser.h
@@ -246,6 +246,9 @@
   /// function call.
   bool CalledSignatureHelp = false;
 
+  /// Parsing a type within __builtin_offsetof.
+  bool InBuiltInOffsetOfBase = false;
+
   /// The "depth" of the template parameters currently being parsed.

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

2022-09-09 Thread YingChi Long via Phabricator via cfe-commits

inclyc created this revision.
Herald added a project: All.
inclyc updated this revision to Diff 459034.
inclyc added a comment.
inclyc added reviewers: aaron.ballman, clang-language-wg.
inclyc published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Apply clang-format


https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm made very
clear that it is an UB having type definitions with in offsetof. After
this patch clang will reject any type definitions in __builtin_offsetof.

Fixes https://github.com/llvm/llvm-project/issues/57065

I'm not sure this is a correct bug fix, because adding a new bool state
to Parser just for supporting this seems ugly. I've tried creating a new
DeclaratorContext, though.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133574

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Parser/declarators.c
  clang/test/Sema/offsetof.c

Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -70,3 +70,16 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+int a;
+struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}}
+{
+  int c;
+  int d;
+} x;
+  }, a);
+}
Index: clang/test/Parser/declarators.c
===
--- clang/test/Parser/declarators.c
+++ clang/test/Parser/declarators.c
@@ -80,10 +80,6 @@
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16254,7 +16254,7 @@
  SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody) {
+ SkipBodyInfo *SkipBody, bool IsWithinOffsetOf) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
@@ -17027,6 +17027,12 @@
cast_or_null(PrevDecl));
   }
 
+  if (IsWithinOffsetOf && TUK == TUK_Definition) {
+Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+<< Context.getTagDeclType(New);
+Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2579,7 +2579,9 @@
   }
   case tok::kw___builtin_offsetof: {
 SourceLocation TypeLoc = Tok.getLocation();
+InBuiltInOffsetOfBase = true;
 TypeResult Ty = ParseTypeName();
+InBuiltInOffsetOfBase = false;
 if (Ty.isInvalid()) {
   SkipUntil(tok::r_paren, StopAtSemi);
   return ExprError();
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2030,7 +2030,7 @@
 DSC == DeclSpecContext::DSC_type_specifier,
 DSC == DeclSpecContext::DSC_template_param ||
 DSC == DeclSpecContext::DSC_template_type_arg,
-);
+, InBuiltInOffsetOfBase);
 
 // If ActOnTag said the type was dependent, try again with the
 // less common call.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -3273,7 +3273,8 @@
  bool , SourceLocation ScopedEnumKWLoc,
  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
- SkipBodyInfo *SkipBody = nullptr);
+ SkipBodyInfo *SkipBody = nullptr,
+ bool IsWithinOffsetOf = false);

73 matches

Mail list logo