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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: MaskRay.
jhenderson added a subscriber: MaskRay.
jhenderson added a comment.

My opinion hasn't changed that it's a mistake to put the dot demangling in llvm-cxxfilt specifically. That will mean you'll need to make the same change in llvm-nm, llvm-readelf, llvm-objdump and probably other tools too. Code duplication is bad. The next tool to be added might not realise that they can't just use the main demangler library as-is to do their name demangling.

If there is tool code that conflicts with that behaviour (such as the llvm-nm mach-o code), perhaps it's that tool code that should change instead, or perhaps we need to enhance the demangler library a little further, for example by adding an optional "strip underscore" behaviour to it. Adding @MaskRay as a reviewer in case he's got a different opinion.



================
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' \
----------------
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!


================
Comment at: llvm/tools/llvm-cxxfilt/llvm-cxxfilt.cpp:71
+
+  // Not consider the prefix dot as part of the demangled symbol name.
+  if (DecoratedStr[0] == '.') {
----------------
Nit: "Do not consider"

Also applies to the patch title.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139864