This is an archive of the discontinued LLVM Phabricator instance.

[llvm-nm] Add test for `--debug-syms --dynamic`
ClosedPublic

Authored by Higuoxing on Mar 15 2020, 11:59 PM.

Details

Summary

This test ensures that llvm-nm will omit NULL symbol.

Diff Detail

Event Timeline

Higuoxing created this revision.Mar 15 2020, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2020, 11:59 PM
Harbormaster completed remote builds in B49279: Diff 250489.
grimar added inline comments.Mar 16 2020, 1:10 AM
llvm/test/tools/llvm-nm/debug-syms.test
47

I am not an expert in llvm-nm, but is debug-syms.test is a right place for this test?
I'd expect to see a test case for --dynamic/-D, but I've not found.
Seems we want to add one instead?

48

You have already tested --debug-syms/-a at the first lines. No need to test them again.
Here I'd expect to see --dynamic/-D.

67

Usually local symbols go first.
Also, STB_LOCAL is the default binding, so you do not need to set it for locals.

68

Is it important to have "Section"?

71

I think you can add DynamicSymbols to the first description and have a single YAML.

Higuoxing updated this revision to Diff 250504.EditedMar 16 2020, 3:27 AM

Addressed comment.

  • Use a single YAML file.
  • Test --dynamic/-D
  • Remove unrelated/unwanted fields
Higuoxing updated this revision to Diff 250527.Mar 16 2020, 5:07 AM

Fix Harbormaster build

Higuoxing marked an inline comment as done.Mar 16 2020, 5:12 AM
Higuoxing added inline comments.
llvm/test/tools/llvm-nm/debug-syms.test
68

Is it important to have "Section"?

I think it's important. We use --implicit-check-not U to detect NULL symbol, so we should specify section field for symbols.

--debug-syms may be legacy cruft of GNU nm. An x86 nm does not hide $a $t mapping symbols. I wonder whether we should drop the auto-hide logic.

--debug-syms may be legacy cruft of GNU nm. An x86 nm does not hide $a $t mapping symbols. I wonder whether we should drop the auto-hide logic.

Mapping symbols are an ARM thing. If we are dumping an ARM object, we should hide the mapping symbols for compatibility (I see people iterating over lists of symbols dumped by nm, so we can't change this behaviour). Obviously, if this isn't an ARM object, they aren't mapping symbols, and should be dumped.

I'm somewhat concerned by the --implicit-check-not U usage to ensure no undefined symbol is printed, as it feels fragile. I'd prefer a more stringent check like --implicit-check-not={{.}}. That will ensure that there is no output other than the checked output.

llvm/test/tools/llvm-nm/debug-syms.test
5–9

Here and below, please add the letter column. This will help ensure the --implicit-check-not U is clearer.

16

No need for two blank lines.

47

I would suggest that a --dynamic dedicated test file makes sense, but that this test case here in this file also makes sense.

grimar added inline comments.Mar 17 2020, 3:02 AM
llvm/test/tools/llvm-nm/debug-syms.test
47

I would suggest that a --dynamic dedicated test file makes sense, but that this test case here in this file also makes sense.

Yeah, I've also came to it. Though we probably should not test -D here s it probably enough to test it once in a " --dynamic dedicated test file"

Addressed comments.

  • Remove test for --debug-syms -D (Leave it to a --dynamic dedicated test file)
  • Use --implicit-check-not {{.}} for a more stringent check.
grimar accepted this revision.Mar 18 2020, 2:11 AM

I have no more comments, this LGTM, thanks! Please wait for other reviewers too.

This revision is now accepted and ready to land.Mar 18 2020, 2:11 AM
jhenderson accepted this revision.Mar 19 2020, 3:08 AM

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

LGTM. @Higuoxing, would you mind writing a separate dedicated test for --dynamic? It would allow for testing the corner cases, which are outside the scope of this test file. It can be a separate patch, no problem.

Of course, I'd like to. Thanks for taking time review!

This revision was automatically updated to reflect the committed changes.

LGTM. You can delete unneeded Phabricator tags with:

% which arcfilter 
arcfilter () {
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

before committing.

LGTM. You can delete unneeded Phabricator tags with:

% which arcfilter 
arcfilter () {
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend -F -
}

before committing.

Oh, thanks a lot! I'll try next time 😆