[PATCH] D99981: [demangler] Support the new Rust mangling scheme (v0)

Thu Apr 22 10:30:41 PDT 2021

dblaikie added inline comments.

================
Comment at: llvm/test/tools/llvm-cxxfilt/rust.test:1
+RUN: llvm-cxxfilt -n  < %s | FileCheck --match-full-lines %s
+
----------------
jhenderson wrote:
> dblaikie wrote:
> > jhenderson wrote:
> > > llvm-cxxfilt tool tests are not the place to test the full library functionality.  - that should be tested in the unittests. This test should contain just enough to cover the behaviour implemented in llvm-cxxfilt itself.
> > I'd disagree here - generally if there's a fairly simple/thin wrapper around a library, we do test it via lit/command line tests rather than unit tests. And that seems to be how llvm-cxxfilt/Demangle is tested currently (most cases seem to be tested in llvm/test/tools/llvm-cxxfilt, some in llvm/test/Demangle (using llvm-cxxfilt), but not much is tested in llvm/unittests/Demangle.
> Not sure I agree with that characterisation. With the exception of the abitag.test test in tools/llvm-cxxfilt, the tests are all primarily testing llvm-cxxfilt level functionality (indirectly some exercise parts of the demangling code too, but I think that's not their purpose).
> 
> I don't mind if it makes sense to test things at a lit level, but they should at least be in llvm/test/Demangle. That being said, a unit test invocation for this should simpler than writing a lit test. See for example `DemangleTest.cpp`, where each case is a single line - a lit test case would require at least two process executions (llvm-cxxfilt and FileCheck), and a FileCheck CHECK pattern, so will inevitably longer. Anecdotally, I also feel like it would be easier to make mistakes in the lit test, although that could be resolved relatively easily by using the right FileCheck flags.
Taking a more detailed look - only looking at itanium manglings since I know how to grep for them easily enough (_Z):

unittests/Demangle/DemangleTest.cpp: 5 mangled names/demanglings
unittests/Demangle/ItaniumDemangleTest.cpp: 1 (presumably a test of API syntax/ergonomics, not of the demangling algorithm)
unittests/Demangle/PartialDemangleTest.cpp: 38 - this is an API designed for novel programmatic use (we use it inside Google for rewriting certain mangled names efficiently for profile matching when core library symbol names change for whatever reason)
test/Demangle: Only contain Microsoft demangling test cases (using llvm-undname, the MS equivalent of llvm-cxxfilt)
test/tools/llvm-cxxfilt: 56 (well, more, since several tests have more than one mangled name per line) mangled names

And especially llvm/test/tools/llvm-cxxfilt/delimiters.test seems to be the most comprehensive piece of demangler algorithm testing, for instance.

The first testing when the library was introduced was done with llvm-cxxfilt ( rGb940b66c6032096d40dfd1859e2598749a30378c ), not a unit test (& that test was initially in llvm/test/Demangle, though it got moved to llvm/tools/llvm-cxxfilt in rG7091820a969ed3503806e28dae371d1b11a9da6d )

This is generally true of LLVM testing - if there's a convenient command line wrapper around the functionality, then lit testing tends to be the focus. (see symbolizer, dwarfdump, objdump, etc)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981