This is an archive of the discontinued LLVM Phabricator instance.

[clang] Merge the SourceRange into ParsedAttributes
ClosedPublic

Authored by tbaeder on Mar 8 2022, 2:31 AM.

Details

Summary

I wasn't 100% whether to add the range to ParsedAttributes or even ParsedAttributesView.

There's still a ParsedAttributesViewWithRange which basically used once in ParseDeclCXX.cpp.

Diff Detail

Event Timeline

tbaeder created this revision.Mar 8 2022, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 2:31 AM
tbaeder requested review of this revision.Mar 8 2022, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 2:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 413744.Mar 8 2022, 2:32 AM
tbaeder added inline comments.
clang/test/SemaOpenCL/address-spaces.cl
261

This is a peculiar ordering problem...

Just a couple of questions/comments. Otherwise this seems pretty ok.

clang/lib/Parse/ParseCXXInlineMethods.cpp
745

This change is concerning, shouldn't ParseGNUAttributeArgs just be losing its 'EndLoc' pointer instead?

clang/lib/Parse/ParseDecl.cpp
1426

Could we lose the endLoc here as well?

clang/lib/Parse/ParseDeclCXX.cpp
3217

This seems like a particularly strange change, What is the reasoning for this? Is it just that the Attrs are unused?

4371

I believe we aren't supposed to be putting the 'name' of the function in the comments anymore, can you just remove it instead of updating it?

clang/lib/Sema/SemaType.cpp
8144

What is the justification for this set of changes? Is it simply to 'early exit' rather than make the unnecessary copy?

erichkeane added inline comments.Mar 11 2022, 8:15 AM
clang/include/clang/Sema/ParsedAttr.h
920

Also, this function is now strange/likely needs a rename, since it likely needs to reset the range as well. I believe the point of this being a separate function is to not clear the 'pool'.

1105–1106

This is... oh boy. I'm hopeful you can remove this type as well.

clang/test/SemaOpenCL/address-spaces.cl
261

can you debug this to see what the problem is? I wouldn't expect the ordering to matter here.

I wasn't 100% whether to add the range to ParsedAttributes or even ParsedAttributesView.

I think ParsedAttributesView is the correct level for this -- I want to see us tracking the source range for all the parsed attributes (we need the information for diagnostics more often than not), and this ensures we always have access to that information.

There's still a ParsedAttributesViewWithRange which basically used once in ParseDeclCXX.cpp.

Any idea why we can't get rid of that one? (Maybe it'll become more clear to me as I review more.)

clang/include/clang/Parse/Parser.h
1583–1592
2071
2310
2314

I'll stop commenting on these -- if you see a function signature where the parsed attribute parameter identifier uses the wrong coding style *and* the rest of the parameters already use the right style, might as well hit those identifiers. No need to fix all naming issues with the touched functions (unless you feel like doing an NFC commit to change them).

clang/lib/Parse/ParseDecl.cpp
500–503

It looks like some unrelated formatting changes snuck in? I'm not opposed, but it's a bit easier to review if this was an NFC commit (before or after landing this patch) because it makes for a smaller diff here. (I noticed this in a few spots, only commenting about it here.)

clang/test/SemaOpenCL/address-spaces.cl
261

I think we need to ensure that we still diagnose both ways, otherwise we'll start to accept invalid code: https://godbolt.org/z/h7473Ehc5

Or does the behavior change here in other ways?

aaron.ballman added inline comments.Mar 11 2022, 8:18 AM
clang/include/clang/Sema/ParsedAttr.h
1105–1106

+1, it'd be fantastic if we could, otherwise we're storing the range twice for this type (and it's named Range both times).

clang/lib/Parse/ParseDeclCXX.cpp
3217

The comment above is that we parse and discard any trailing attributes, so this change is scoping the ParsedAttributes object more tightly to the only scope it's needed, but otherwise not changing behavior.

tbaeder marked 3 inline comments as done.Mar 15 2022, 11:51 PM
tbaeder added inline comments.
clang/lib/Parse/ParseCXXInlineMethods.cpp
745

IIRC this is just a cleanup - as you can see, the local endLoc is unused apart from being passed to those two function calls, which can handle nullptr just fine.

clang/lib/Parse/ParseDeclCXX.cpp
3217

Yup, both right. The change is not strictly necessary but makes sense IMO.

clang/lib/Sema/SemaType.cpp
8144

It avoids a copy and made debugging the opencl ordering problem a bit easier as the loop below is not reached for empty attribute lists. I can leave it out of this patch if you want.

clang/test/SemaOpenCL/address-spaces.cl
261

I investigated this a bit, which is why I know it's an ordering problem. Let's see...

The "multiple address spaces specified for type" error is emitted both before and after this patch, so this is about the second warning about function scope variables. That warning is emitted in Sema::CheckVariableDeclarationType() if the address space of the type is LangAS::opencl_global.

In HandleAddressSpaceTypeAttribute() in SemaType.cpp, the "multiple address spaces specified for type" error is emitted, which marks the second attribute invalid and ignores it. So if the __attribute__((opencl_global)) is handled second, it's ignored and won't reach CheckVariableDeclarationType(), where the second error is emitted.

tbaeder marked 7 inline comments as done.Mar 16 2022, 1:37 AM
tbaeder added inline comments.
clang/include/clang/Sema/ParsedAttr.h
1105–1106

With the range in ParsedAttributesView, this is indeed not needed.

clang/lib/Parse/ParseDecl.cpp
500–503

I've tried reverting some of them, but git clang-format adds them back every time...

1426

Possibly. There are still a few of the attribute parsing functions left that take an endLoc (esp. when it comes to attribute args). I can look into that after this patch, it's big enouhg as it is I think.

tbaeder updated this revision to Diff 415729.Mar 16 2022, 1:39 AM
tbaeder marked an inline comment as done.

Remove ParsedAttributesViewWithRange and address other review comments.

tbaeder updated this revision to Diff 415757.Mar 16 2022, 3:14 AM
aaron.ballman accepted this revision.Mar 16 2022, 6:02 AM

Just NFC nits from me with coding style stuff, so this LGTM! Feel free to fix the style nits when you land or land this as-is and do an NFC commit to fix them.

clang/include/clang/Parse/Parser.h
2770
3008
clang/include/clang/Sema/DeclSpec.h
2516–2521
clang/lib/Parse/ParseDecl.cpp
500–503

Huh, that's very odd!

1426
clang/lib/Parse/ParseDeclCXX.cpp
474
clang/test/SemaOpenCL/address-spaces.cl
261

Ah, thank you for this -- so both situations are diagnosed correctly, it's just a matter that cascading error behavior changed. I'm fine with that.

This revision is now accepted and ready to land.Mar 16 2022, 6:02 AM
tbaeder updated this revision to Diff 417235.Mar 22 2022, 3:41 AM
tbaeder marked 5 inline comments as done.
tbaeder added inline comments.
clang/include/clang/Sema/DeclSpec.h
2516–2521

I blindly changed this and it took me a while to figure out that's wrong from the test failures:

Attrs.takeAllFrom(Attrs)...

clang/include/clang/Sema/ParsedAttr.h
920

Clearing the source range in here causes a bunch of test failures FWIW, I assume because the old ParsedAttributesWithRange did it in clean() as well.

aaron.ballman added inline comments.Mar 22 2022, 7:43 AM
clang/include/clang/Sema/DeclSpec.h
2516–2521

Oh god, I'm so sorry for that terrible suggestion, I hadn't spotted I was reusing the name. Feel free to go with A or PA or something for the parameter name to avoid that conflict.

tbaeder marked an inline comment as done.Mar 22 2022, 8:11 AM
tbaeder added inline comments.
clang/include/clang/Sema/DeclSpec.h
2516–2521

Haha, no problem. Do you think adding an assertion for this case to takeAllFrom() (and takeOneFrom()) makes sense?

aaron.ballman added inline comments.Mar 22 2022, 8:15 AM
clang/include/clang/Sema/DeclSpec.h
2516–2521

An assertion that the attributes are actually taken from the argument (so validating the size of the container after taking from it)? Probably wouldn't hurt.

erichkeane added inline comments.Mar 22 2022, 8:18 AM
clang/include/clang/Sema/DeclSpec.h
2516–2521

I'd suggest more of a 'Make sure other side is not this side' :D

clang/include/clang/Sema/ParsedAttr.h
813

so: assert(&pool != this && "Stealing from yourself? Super bad...").

tbaeder marked an inline comment as done.Mar 22 2022, 8:44 AM
tbaeder added inline comments.
clang/include/clang/Sema/ParsedAttr.h
813

RIght, that's what I had locally.

tbaeder updated this revision to Diff 417509.Mar 22 2022, 11:28 PM
aaron.ballman accepted this revision.Mar 23 2022, 5:09 AM

LGTM, feel free to add the assert when you land.

clang/include/clang/Sema/ParsedAttr.h
813

That's a reasonable assert to add.

This revision was landed with ongoing or failed builds.Mar 24 2022, 12:12 AM
This revision was automatically updated to reflect the committed changes.