This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jhenderson on Dec 9 2019, 6:05 AM.

Details

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.

Diff Detail

Event Timeline

jhenderson created this revision.Dec 9 2019, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 6:05 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
jhenderson retitled this revision from [test][llvm-cxxfilt] to [test][llvm-cxxfilt] Fix darwin build bot/improve test naming and commenting.Dec 9 2019, 6:06 AM
jhenderson edited the summary of this revision. (Show Details)
grimar added a comment.Dec 9 2019, 6:18 AM

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 marked 2 inline comments as done.Dec 9 2019, 7:12 AM
jhenderson added inline comments.
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 added inline comments.Dec 9 2019, 7:39 AM
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 marked an inline comment as done.Dec 9 2019, 7:58 AM
jhenderson added inline comments.
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 added inline comments.Dec 9 2019, 10:08 PM
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 marked an inline comment as done.EditedDec 10 2019, 8:08 AM

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.

jhenderson marked an inline comment as done.Dec 10 2019, 8:08 AM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 10 2019, 8:10 AM
This revision was automatically updated to reflect the committed changes.