CPlusPlusNameParser does not handles templated operator< properly

[shafik]

Summary

CPlusPlusNameParser is used in several places on of them is during IR execution and setting breakpoints to pull information C++ like the basename, the context and arguments.

Currently it does not handle templated operator< properly. It used clang::Lexer which will tokenize operator<<A::B> into:

tok::kw_operator
tok::lessless
tok::raw_identifier

Later on the parser in ConsumeOperator() does not handle this case properly and we end up failing to parse.

[shafik]

My fix in ConsumeOperator() is not proper but if everyone feels this is correct approach I will create member functions to deal with this cleanly.

Other approaches could be modifying ExtractTokens() to detect this case and generate two tok::lessin place of tok::lessless but this feels like the wrong place to fix this. I tried to understand how clang handles this case since but it was not obvious. AFAICT they have to deal with this case too.

[labath]

The approach seems reasonable to me.

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

153–155

You could add these to the "EXPECT_FALSE" list below

[hubert.reinterpretcast]

I am not sure what the usage scenario is that this is meant to support. Is it user input that tries to name a specialization of a template operator< without separation to prevent tokenization as operator<<? I think that case should qualify as user error.

Both C++17 subclause 17.2 [temp.names] and N4849 subclause 13.3 [temp.names] imbue the "angle-bracket" treatment of < to after identification of a name (not to the formation of the name itself) and does involve special treatment of <<. Special treatment in tokenization (C++17 subclause 5.4 [lex.pptoken]; N4849 subclause 5.4 [lex.pptoken]) for angle brackets extends to <: and not to <<.

All of Clang, GCC, ICC, and MSVC do not compile the following:

enum E { E0 };
template <typename T> void operator<(E, T);

void g() {
  operator<<int>(E0, 0);  // does not compile
}

Compiler Explorer (godbolt.org) >>link<<.

In other words, where is the input coming from? Should the producer of the input be corrected instead?

[shafik]

In D76168#1924539, @hubert.reinterpretcast wrote:

I am not sure what the usage scenario is that this is meant to support. Is it user input that tries to name a specialization of a template operator< without separation to prevent tokenization as operator<<? I think that case should qualify as user error.

Both C++17 subclause 17.2 [temp.names] and N4849 subclause 13.3 [temp.names] imbue the "angle-bracket" treatment of < to after identification of a name (not to the formation of the name itself) and does involve special treatment of <<. Special treatment in tokenization (C++17 subclause 5.4 [lex.pptoken]; N4849 subclause 5.4 [lex.pptoken]) for angle brackets extends to <: and not to <<.

All of Clang, GCC, ICC, and MSVC do not compile the following:

enum E { E0 };
template <typename T> void operator<(E, T);

void g() {
  operator<<int>(E0, 0);  // does not compile
}

Compiler Explorer (godbolt.org) >>link<<.

In other words, where is the input coming from? Should the producer of the input be corrected instead?

Hubert, you are indeed correct. I hit this realization when I was going to sleep yesterday.

This is an unfortunate oddity of debug info. When clang generates DW_AT_Names for templates it appends the template parameters to them, gcc also does this BTW.

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

[labath]

In D76168#1925176, @shafik wrote:

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

So is this some alternative to D75761 (where we'd use CPlusPlusNameParser to decode DW_AT_names of templates)? If so, I think that is an interesting direction, but beware that that class is kind of meant for processing the demangler output. The contents of DW_AT_name looks a bit like a demangled name, but in reality there are some deviations from that format (which is why I did not recommend this direction initially -- but I am not against it either).

Also it looks like llvm and gnu demanglers disagree on the exact formatting of demangled operator names:

$ c++filt _ZlsI1AEvT_S1_
void operator<< <A>(A, A)
$ llvm-cxxfilt _ZlsI1AEvT_S1_
void operator<<<A>(A, A)

I think the gnu format is superior (and unambiguous) so we could change llvm to match that -- and this change probably won't require any kind of compatibility hacks.

[shafik]

In D76168#1929211, @labath wrote:

In D76168#1925176, @shafik wrote:

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

So is this some alternative to D75761 (where we'd use CPlusPlusNameParser to decode DW_AT_names of templates)? If so, I think that is an interesting direction, but beware that that class is kind of meant for processing the demangler output. The contents of DW_AT_name looks a bit like a demangled name, but in reality there are some deviations from that format (which is why I did not recommend this direction initially -- but I am not against it either).

Also it looks like llvm and gnu demanglers disagree on the exact formatting of demangled operator names:

$ c++filt _ZlsI1AEvT_S1_
void operator<< <A>(A, A)
$ llvm-cxxfilt _ZlsI1AEvT_S1_
void operator<<<A>(A, A)

I think the gnu format is superior (and unambiguous) so we could change llvm to match that -- and this change probably won't require any kind of compatibility hacks.

This is not an alternative, this is a complement to that fix. So even with D75761 we still fail in expressions and setting breakpoints for symbols of that type. So both fixes are needed.

I personally would like to stop emitting template parameters for DW_AT_names but I know this will break some stuff and I have to see if it is workable or not, I think it is.

Lastly, we will still need to support older compilers regardless.

[labath]

In D76168#1929733, @shafik wrote:

In D76168#1929211, @labath wrote:

In D76168#1925176, @shafik wrote:

Long-term I would like to modify clang to stop doing that for LLDB, but LLDB will still have to support older compilers for a while. So I think this fix is still needed.

So is this some alternative to D75761 (where we'd use CPlusPlusNameParser to decode DW_AT_names of templates)? If so, I think that is an interesting direction, but beware that that class is kind of meant for processing the demangler output. The contents of DW_AT_name looks a bit like a demangled name, but in reality there are some deviations from that format (which is why I did not recommend this direction initially -- but I am not against it either).

Also it looks like llvm and gnu demanglers disagree on the exact formatting of demangled operator names:

$ c++filt _ZlsI1AEvT_S1_
void operator<< <A>(A, A)
$ llvm-cxxfilt _ZlsI1AEvT_S1_
void operator<<<A>(A, A)

I think the gnu format is superior (and unambiguous) so we could change llvm to match that -- and this change probably won't require any kind of compatibility hacks.

This is not an alternative, this is a complement to that fix. So even with D75761 we still fail in expressions and setting breakpoints for symbols of that type. So both fixes are needed.

I personally would like to stop emitting template parameters for DW_AT_names but I know this will break some stuff and I have to see if it is workable or not, I think it is.

Lastly, we will still need to support older compilers regardless.

sounds good to me.

[aprantl]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp

333

Add a comment what this block is doing?

334

Thinking out loud: This is looking ahead two tokens.

342

here we are pushing an artificial less token, but it's not obvious why what works, so if you could add a comment explaining what is going on here, that would help.

[shafik]

Addressing comments:

• Adding more detailed comments
• Adding test for cases that currently fail b/c we don't enable C++20

[aprantl]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp

333

"we" is super ambiguous. This is LLDB, so you may want to say Clang and/or GCC does this.

339

we with then

343

.

345

.

347

.

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

147

... how Clang generates debug info?

[shafik]

Fixing up loose language in the comments and adding periods.

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp

bool CPlusPlusNameParser::ConsumeOperator() {
   if (!ConsumeToken(tok::kw_operator))
     return false;
 
   if (!HasMoreTokens()) {
     return false;
   }
 
   const auto &token = Peek();
+
+  // When we generate debug info we add template parameters to names.
+  // Since we don't add a space between the name and the template parameter in
+  // some cases we are not generating valid C++ names e.g.:
+  //
+  //   operator<<A::B>
+  //
+  // In some cases we with then parse incorrectly. This fixes the issue by
+  // detecting this case and inserting tok::less in place of tok::lessless
+  // and returning successfully that we consumed the operator.
+  if (token.getKind() == tok::lessless) {
+    // Make sure we have more tokens before attempting to look ahead one more
+    if (m_next_token_index + 1 < m_tokens.size()) {
+      // Look ahead two tokens
+      clang::Token n_token = m_tokens[m_next_token_index + 1];
+      // If we find ( or < then this is indeed operator<< no need for fix
+      if (n_token.getKind() != tok::l_paren && n_token.getKind() != tok::less) {
+        clang::Token tmp_tok;
+
+        tmp_tok.setLength(1);
+        tmp_tok.setLocation(token.getLocation().getLocWithOffset(1));
+        tmp_tok.setKind(tok::less);
+
+        m_tokens[m_next_token_index] = tmp_tok;
+
+        start_position.Remove();
+        return true;
+      }
+    }
+  }
+
   switch (token.getKind()) {
   case tok::kw_new:
   case tok::kw_delete:
     // This is 'new' or 'delete' operators.
     Advance();
     // Check for array new/delete.
     if (HasMoreTokens() && Peek().is(tok::l_square)) {
       // Consume the '[' and ']'.

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

TestCase test_cases[] = {
       {"std::vector<int>::push_back", "std::vector<int>", "push_back"},
       {"operator<<(Cls, Cls)::Subclass::function",
        "operator<<(Cls, Cls)::Subclass", "function"},
       {"std::vector<Class, std::allocator<Class>>"
        "::_M_emplace_back_aux<Class const&>",
        "std::vector<Class, std::allocator<Class>>",
        "_M_emplace_back_aux<Class const&>"},
       {"`anonymous namespace'::foo", "`anonymous namespace'", "foo"},
-      {"`operator<<A>'::`2'::B<0>::operator>",
-       "`operator<<A>'::`2'::B<0>",
+      {"`operator<<A>'::`2'::B<0>::operator>", "`operator<<A>'::`2'::B<0>",
        "operator>"},
       {"`anonymous namespace'::S::<<::__l2::Foo",
-       "`anonymous namespace'::S::<<::__l2",
-       "Foo"}};
+       "`anonymous namespace'::S::<<::__l2", "Foo"},
+      // These cases are idiosyncratic to how we generate debug info for names
+      // when we have template parameters. They are not valid C++ names but if
+      // we fix this we need to support them for older compilers.
+      {"A::operator><A::B>", "A", "operator><A::B>"},
+      {"operator><A::B>", "", "operator><A::B>"},
+      {"A::operator<<A::B>", "A", "operator<<A::B>"},
+      {"operator<<A::B>", "", "operator<<A::B>"},
+      {"A::operator<<<A::B>", "A", "operator<<<A::B>"},
+      {"operator<<<A::B>", "", "operator<<<A::B>"},
+  };
 
   llvm::StringRef context, basename;
   for (const auto &test : test_cases) {
     EXPECT_TRUE(CPlusPlusLanguage::ExtractContextAndIdentifier(
         test.input.c_str(), context, basename));
     EXPECT_EQ(test.context, context.str());
     EXPECT_EQ(test.basename, basename.str());
   }
 
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier("void", context,
                                                               basename));
   EXPECT_FALSE(
       CPlusPlusLanguage::ExtractContextAndIdentifier("321", context, basename));
   EXPECT_FALSE(
       CPlusPlusLanguage::ExtractContextAndIdentifier("", context, basename));
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
       "selector:", context, basename));
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
       "selector:otherField:", context, basename));
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
       "abc::", context, basename));
   EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
       "f<A<B><C>>", context, basename));
+
+  // We expect these cases to fail until we turn on C++2a
+  EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
+      "A::operator<=><A::B>", context, basename));
+  EXPECT_FALSE(CPlusPlusLanguage::ExtractContextAndIdentifier(
+      "operator<=><A::B>", context, basename));
 }
 
 static std::set<std::string> FindAlternate(llvm::StringRef Name) {
   std::set<ConstString> Results;
   uint32_t Count = CPlusPlusLanguage::FindAlternateFunctionManglings(
       ConstString(Name), Results);
   EXPECT_EQ(Count, Results.size());
   std::set<std::string> Strings;