[PATCH] D139864: [llvm-cxxfilt] Do not consider the prefix dot as part of the demangled symbol name.

Fri Sep 22 01:24:18 PDT 2023

jhenderson added a comment.

Could you not just modify `nonMicrosoftDemangle` to handle the dot prefix? That would mean this works for all tools, including llvm-cxxfilt, rather than needing this and the separate D159539 <https://reviews.llvm.org/D159539> patch. I'm assuming that a dot prefix in front of a Microsoft mangled name doesn't make sense...

================
Comment at: llvm/test/tools/llvm-cxxfilt/delimiters.test:36
+RUN:      '_Z3Foo$ _Z3f.0v' \
+## Show that the llvm-cxxfilt does not consider the prefix dot as part of the demangled symbol name.
+RUN:      '._Z3Foo ._Z3f.0v' \
----------------
DiggerLin wrote:
> jhenderson wrote:
> > That being said, this isn't the right place for these new test cases. The only thing that should change in this test is related to the `._Z3Foo` string - see below.
> > 
> > This test is about testing which characters are treated as delimiters, i.e. characters that separate sequences of names that are to be demangled individually. It is not about testing how things are demangled in the presence of ".". You should add that testing in a new test file.
> > 
> > With the change in behaviour of leading dot prefixes, this test itself doesn't actually test the intended behaviour. The intention was to show that a dot was treated as part of the mangled name, rather than a separator between the previous character sequence (in this case simply the preceding space), and the subsequent mangled name (`_Z3Foo`). If the dot was treated as a delimiter, the `_Z3Foo` would have been demangled, resulting in the final part of the output being `_Z3Foo$ .Foo`. As you can see, this is indistinguishable from the case where `._Z3Foo` as a whole is demangled (to `.Foo`). Instead, this bit of the bit of the check should be modified such that the new code can't demangle it still, but if instead the dot was treated as delimiter, it could be. For example, you could have two valid name manglings separated by a single ".", e.g. `_Z3Foo._Z3Bar`. If the dot were a delimiter, that would result in an output of `Foo.Bar`, but since it isn't, the original input should be emitted to the output (i.e. `_Z3Foo._Z3Bar`).
> > 
> > I hope that makes sense!
> > The intention was to show that a dot was treated as part of the mangled name
> 
> `._Z3Foo` is a symbol name in AIX , but the  `._Z3Foo` is not a mangle name ,only  `_Z3Foo` is a mangle name, we put the dot in front of the `_Z3Foo` to generate the entry label symbol of function in AIX.
> 
> 
> 
> > but if instead the dot was treated as delimiter, it could be. For example, you could have two valid name manglings separated by a single ".", e.g. _Z3Foo._Z3Ba
> 
>  according to the https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling , the dot maybe part of symbol (but in the middle of the mangled name, not the front of the mangle name).
> 
> So we have to do some special for the front dot of symbol name for AIX.
> > The intention was to show that a dot was treated as part of the mangled name
> 
> `._Z3Foo` is a symbol name in AIX , but the  `._Z3Foo` is not a mangle name ,only  `_Z3Foo` is a mangle name, we put the dot in front of the `_Z3Foo` to generate the entry label symbol of function in AIX.

You're arguing about semantics here. As far as I'm concerned, the dot is part of the mangled name. The demangling code removes it, demangles the rest of the name, and then readds it, but it's still part of the mangled name (just not part of the Itanium mangling).

But regardless of the exact terminology, it's clear you didn't understand my comment. We both agree that the dot is part of the name (whether it's part of the mangled name or not is irrelevant in this context). What's important to this test is that it isn't a delimiter (i.e. a character that separates a string into two separate names) and the rest of my comment and explanation still stands: your change in behaviour has rendered this part of the test as no longer testing what it wanted to test and therefore the test needs updating as I already explained (and you need proper testing of the dot demangling in a different test, which I see you've added).

================
Comment at: llvm/test/tools/llvm-cxxfilt/prefix-dot.test:1
+## Show that the llvm-cxxfilt does not consider the prefix dot to be part of the symbol name to be demangled.
+
----------------
I suggest we use the phrase "dot prefix" rather than "prefix dot" in the test name and comment as it's more natural. Same goes in other places in this patch.

================
Comment at: llvm/test/tools/llvm-cxxfilt/prefix-dot.test:3-4
+
+RUN: echo '._Z3Foo ._Z3f.0v' \
+RUN:      '._ZL5func0v ._Z5func1i' > %t
+RUN:      llvm-cxxfilt -n < %t | FileCheck %s
----------------
I don't think you need this many strings to show the behaviour. One case where it can be successfully demangled and one case where it cannot be demangled I think should be sufficient.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139864