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

Fri Apr 23 01:13:30 PDT 2021

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-cxxfilt/rust.test:1
+RUN: llvm-cxxfilt -n  < %s | FileCheck --match-full-lines %s
+
----------------
dblaikie wrote:
> 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)
We could go back and forth on this a while, I feel.

`delimiters.test` may have multiple manglings, but the demangling behaviour isn't what's being tested in that test. The purpose of that test is to ensure that the right non-alphanumeric characters act as a delimiter between two character sequences for llvm-cxxfilt's demangling process (see rG5cd5f8f2563395f8767f94604eb4c4bea8dcbea0 where it was introduced). Note that actually it's the same very simple mangled name that is demangled in every case. This behaviour is entirely within the llvm-cxxfilt tool code, not the library code.

There are similar stories for most of the other tests in tools/llvm-cxxfilt too. In general, it looks like there is very little dedicated testing of the Itanium demangler code, which is probably part of the problem here.

In other GNU binutils replacements, we try to ensure tests that are testing the underlying library code are in test/<library name> rather than test/tool/<tool name>. It's not perfect, but it has been a goal. The idea is that developers of a library that is used by multliple tools can find the tests for that library without having to go through potentially multiple different test/tool/ directories. It also helps the test directory structure reflect the llvm code structure better. Example: if I made a change to a Support class, I'd usually add my test to test/Support, if unit tests aren't viable.

> 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)

If I understand it correctly, this is partly for legacy reasons - unit tests haven't been around in LLVM for all that long, so people use what they are used to. It's also because it's often easier to write the lit test than it is to get the setup right for a unit test, which is perfectly reasonable to me. That being said, I'm not opposed to using lit tests instead of unit tests, if the developer prefers (as long as the lit tests are in the right place), although it's worth pointing out that if this patch were to be split up, llvm-cxxfilt support would probably belong in a separate patch, and therefore testing would HAVE to be put into the unit tests.

Maybe I'm not reflecting the wider LLVM view? Should this be brought up on llvm-dev?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981