[test][llvm-cxxfilt] Fix darwin build bot/improve test naming and commenting

[jhenderson]

Summary

When committing rGdba420bc05ae, I missed that a darwin-specific change had been recently introduced into llvm-cxxfilt (see rGe84468c1f145), which my change ignored and consequently broke the darwin build bot. This change fixes this issue as well as improving naming/commenting of things related to this point so that people are less likely to run into the same issue as I did.

I have landed this patch already as rG281539053238 to unbreak the build bot, but would appreciate a post-commit review.

[grimar]

LG. I have only a single suggestion.

llvm/test/tools/llvm-cxxfilt/simple.test

9

It is probably not obvious that "-n" is the same as --strip-underscore (help text does not show it).
(When I read "Note that this test uses "-n" because on darwin, the default for --strip-underscore is true"
I wasn't sure if -n is the same as --strip-underscore or it was a mistype/copy-paste error)

I'd try stick to the options that are listed in a help text in out tests probably.

[jhenderson]

llvm/test/tools/llvm-cxxfilt/simple.test

1–7

This line should have llvm-cxxfilt -n. I've fixed this in rG01d8bb49.

9

-n is what was there before, and is probably better due to being shorter overall. I don't know why it wasn't added to the help text, but hopefully future changes to fix the command-line printing of aliases should fix that. I'll update the comment for additional clarity though.

[compnerd]

llvm/test/tools/llvm-cxxfilt/strip-underscore-default.test

8 ↗

(On Diff #232828)

Can you merge this into the previous test and use -triple to test the defaults? That ensures that the tests always run and people on other hosts can validate that they do not regress tins things.

[jhenderson]

llvm/test/tools/llvm-cxxfilt/strip-underscore-default.test

8 ↗

(On Diff #232828)

Sounds reasonable. I'll see what I can figure out with that.

[MaskRay]

llvm/test/tools/llvm-cxxfilt/strip-underscore-default.test

8 ↗

(On Diff #232828)

The output of sys::getProcessTriple() is fixed (inferred from LLVM_HOST_TRIPLE).

static bool shouldStripUnderscore() {
  if (StripUnderscore)
    return true;
  if (NoStripUnderscore)
    return false;
  // If none of them are set, use the default value for platform.
  // macho has symbols prefix with "_" so strip by default.
  return Triple(sys::getProcessTriple()).isOSBinFormatMachO();
}

UNSUPPORTED: system-darwin
REQUIRES: system-darwin

Seem the best to test the platform differences.

[jhenderson]

I've committed rG9614a7c9391 with the proposed comment update. Please let me know if anybody wants any other changes to rG281539053238.

llvm/test/tools/llvm-cxxfilt/strip-underscore-default.test

8 ↗

(On Diff #232828)

Right, as @MaskRay says, --triple isn't an option, since the LLVM_HOST_TRIPLE definition is defined at build time. Additionally, the --triple option isn't available in llvm-cxxfilt. It's a specific option for only certain tools e.g. llvm-objdump and llvm-mc.

llvm/test/tools/llvm-cxxfilt/simple.test

 ## Show that llvm-cxxfilt can handle basic demangling for command-line inputs,
 ## including when they are part of a string. Note that this test uses "-n"
-## because on darwin, the default for --strip-underscore is true, but false
-## elsewhere.
+## (alias for --no-strip-underscore) because on darwin, the default for
+## --strip-underscore is true, but false elsewhere.
 
 RUN: llvm-cxxfilt -n _Z1fi abc | FileCheck %s
 RUN: echo "Mangled _Z1fi and _Z3foov in string." | llvm-cxxfilt -n \
 RUN:   | FileCheck %s --check-prefix=CHECK-STRING
 RUN: llvm-cxxfilt -n "CLI remains mangled _Z1fi" \
 RUN:   | FileCheck %s --check-prefix=CHECK-MANGLED
 
 CHECK: f(int)
 CHECK-NEXT: abc
 CHECK-STRING: Mangled f(int) and foo() in string.
 CHECK-MANGLED: CLI remains mangled _Z1fi