[PATCH] D99861: [Clang] Record tokens in attribute arguments for user-defined C++/C2x attributes

[Josh Junon via Phabricator via cfe-commits]

Qix- planned changes to this revision.
Qix- marked 5 inline comments as done.
Qix- added inline comments.


================
Comment at: clang/examples/PrintAttributeTokens/CMakeLists.txt:3-10
+if( NOT MSVC ) # MSVC mangles symbols differently, and
+               # PrintAttributeTokens.export contains C++ symbols.
+  if( NOT LLVM_REQUIRES_RTTI )
+    if( NOT LLVM_REQUIRES_EH )
+      set(LLVM_EXPORTED_SYMBOL_FILE ${CMAKE_CURRENT_SOURCE_DIR}/PrintAttributeTokens.exports)
+    endif()
+  endif()
----------------
aaron.ballman wrote:
> I'm not certain what this is actually doing -- the .exports file is empty (and no other plugin has this sort of .exports file thing). Is this needed?
Most likely not - the directory was copied from the PrintFunctionNames directory, which also has an empty exports file. I'm not completely familiar with the LLVM build configs so I figured it was required. Happy to remove it if need be :)


================
Comment at: clang/examples/PrintAttributeTokens/PrintAttributeTokens.cpp:37
+                                   const ParsedAttr &Attr) const override {
+    llvm::errs() << "PrintAttributeTokens -----------------------\n";
+    D->dump(llvm::errs());
----------------
aaron.ballman wrote:
> Should probably use `llvm::outs()` instead (here and elsewhere in the plugin).
Sure thing, though should PrintFunctionNames be updated as well? Mostly copied the conventions over from that.


================
Comment at: clang/examples/PrintAttributeTokens/PrintAttributeTokens.cpp:48-50
+        if (tokens[i].isLiteral()) {
+          llvm::errs() << "\t=" << tokens[i].getLiteralData();
+        }
----------------
aaron.ballman wrote:
> It'd probably be handy to also print identifier tokens.
Yeah I caught that the other day too, thanks for reminding me.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:284
+  Token *getTokensBuffer() { return getTrailingObjects<Token>(); }
+  Token const *getTokensBuffer() const { return getTrailingObjects<Token>(); }
+
----------------
aaron.ballman wrote:
> 
Agreed; should 279 be updated too?


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:486
+  const Token *getTokens() const {
+    assert(NumTokens > 0 && "No Tokens to retrieve!");
+    return getTokensBuffer();
----------------
aaron.ballman wrote:
> I think it'd be reasonable to return `nullptr` in this case, WDYT?
Sure, that's fine. I was trying to think why I did the assertion but nullptr seems fine too.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4100-4102
+  // directly. The recording happens here because this is the only place
+  // where user-defined (via plugins) attributes are parsed, and thus
+  // they care about the token stream directly.
----------------
aaron.ballman wrote:
> I think plugins will expect these tokens to be available regardless of which attribute syntax is used, so you may need to look into also doing work for GNU and declspec attribute argument lists.
As far as I understand (and perhaps poorly communicated in the comment) is that plugin-defined attributes will always hit this codepath as opposed to the built-in attribute parsers.

I could be wrong here, though. Are arbitrary tokens even allowed in GNU/Declspec attributes? I thought it was just the C2x/C++ attributes syntax that allowed non-identifier tokens in the first place.

Either way, from what I could tell (trying a few different implementations of this change), this is the only place where user-defined attributes are parsed. I certainly could be missing something though.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:4121-4122
+    // Add new attribute with the token list.
+    // We assert that we have at least one token,
+    // since we have to ignore the final r_paren.
+    assert(RecordedTokens.size() > 0);
----------------
aaron.ballman wrote:
> To be clear, we should have at least *two* tokens, right? One for the left paren and one for the right?
This was confusing for me too at first.

By 4112, the left paren token has already been emitted by the lexer; `ConsumeParen` advances to the next token *after* the left-paren; _that_ token then becomes the first in the recording session.

Further, `SkipUntil(tok::r_paren)` checks if the current token is an `r_paren`, and if not, advances and then starts over. It doesn't advance _then_ check.

It's the presence of the left paren that advances us to this stage of the parsing in the first place and thus is emitted before our recording session starts. An empty argument list (`[[some_attr()]]`) thus only has an `r_paren` token emitted to the recording session, which is then ignored.

This is the existing design of the lexer it seems - I wasn't expecting the "reversed" order of operations, and the names of the lexer methods didn't help, either :)


================
Comment at: clang/test/Frontend/plugin-print-attr-tokens.cpp:9-12
+[[plugin::print_tokens("a string")]] void fn1b() {}
+[[plugin::print_tokens()]] void fn1c() {}
+[[plugin::print_tokens(some_ident)]] void fn1d() {}
+[[plugin::print_tokens(int)]] void fn1e() {}
----------------
aaron.ballman wrote:
> Can  you also verify that the token stream output from the plugin looks correct?
> 
> Can you also add additional tests for GNU and declspec attribute syntaxes?
> Can you also verify that the token stream output from the plugin looks correct?

Absolutely; how do I do this? The attributes don't make it into the final AST as far as I can tell, so is there a filecheck mechanism for checking the stdio streams directly? Is there an existing test that can do this I can reference?

> Can you also add additional tests for GNU and declspec attribute syntaxes?

See comment above about GNU/declspec syntaxes. Depending on the outcome there, I'll certainly add them here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99861