Page MenuHomePhabricator

[llvm-readobj][MachO] Add option to sort the symbol table before dumping (MachO only, for now).
ClosedPublic

Authored by oontvoo on Jan 6 2022, 7:41 PM.

Details

Summary

This would help making tests less brittle as the order will be fixed.

(see also PR/53026)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Mar 28 2022, 12:17 AM
llvm/tools/llvm-readobj/MachODumper.cpp
627

Yeah, I understand the concern. The downside with keeping it separate is that you have to effectively repeat the switch/case involved in option parsing in two places, which leads to ugly warts like the need for the assert(false) in the second one. Up to you though.

648

I might be mistaken, but I believe LLVM usually doesn't bother commenting out unused parameter names.

llvm/tools/llvm-readobj/ObjDumper.h
100
llvm/tools/llvm-readobj/llvm-readobj.cpp
91

UNSUPPORTED or UNKNOWN instead of UNSPEC.

360

llvm::Optional would be the more expressive form here. You could then pass it directly, rather than via the pointer, and have a None check instead of a nullptr check.

372

Here and below, I don't think you need the trailing return types.

381

Use case UNSPEC (renamed according to my earlier comment) here, instead of default, to take advantage of compiler warnings about not all cases being filled. See also https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations.

382

Use llvm_unreachable rather than assert(false).

387
  1. Error and warning messages shouldn't end with a "."
  2. I have concerns here: this will stop dumping as soon as an unsupported file type is encountered (even in an archive), yet that is unlikely what the user wants to happen. They more likely want to continue dumping and just not sort in this case (imagine if they had mutliple different file format objects in their input). This should be at most a warning (including the input file name), although I could even see an argument for not having it at all. It also needs testing.
oontvoo marked 8 inline comments as done.Mar 28 2022, 8:09 AM
oontvoo added inline comments.
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
24–32

the comment about white was out of date (no longer needed)

llvm/tools/llvm-readobj/MachODumper.cpp
627

(ack'ed - keeping this as is for now until other format started implementing the option)

llvm/tools/llvm-readobj/llvm-readobj.cpp
387
  1. Fixed
  2. If the format doesn't support --sort-symbols then the users shouldn't specify --sort-symbols. (Keeping it as error for consistency - similarly to how it handles unknown sort-key above, which is that if it doesn't understand it, then it's an error, rather than trying to guess. This file also doesn't have precedence for "warning")
oontvoo updated this revision to Diff 418592.Mar 28 2022, 8:09 AM
oontvoo marked 2 inline comments as done.

addressed review comment

jhenderson added inline comments.Mar 28 2022, 8:21 AM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
24–32

You can still simplify the checks as described in my original comment.

llvm/tools/llvm-readobj/MachODumper.cpp
648

Unaddressed but marked as done?

llvm/tools/llvm-readobj/llvm-readobj.cpp
360

I was mistaken to put the llvm:: qualifier before Optional. It looks like many (most?) instances don't use the qualifier for it or None, so please remove it.

372

Not addressed?

387

You're making the potentially incorrect assumption that all input are of the same format. If a user has two objects of different formats, they might want the symbols sorted for the ones that can be:

$ llvm-readobj elf.o macho.o --sort-symbols --symbols

Would result in an error, and nothing printed (not even macho.o's symbols).

oontvoo updated this revision to Diff 418603.Mar 28 2022, 8:49 AM
oontvoo marked 3 inline comments as done.

updated diff

oontvoo marked an inline comment as done.Mar 28 2022, 8:49 AM
oontvoo marked 2 inline comments as done.

Looks good aside from the test comment.

llvm/tools/llvm-readobj/llvm-readobj.cpp
387–390

This warning is untested. Unless you have patches lined up for all other formats, I'd add a test that shows what happens for a mixture of sortable and unsortable formats, e.g. llvm-readobj wasm.o macho.o elf.o

oontvoo updated this revision to Diff 418883.Mar 29 2022, 8:07 AM

added test for warning case

oontvoo marked an inline comment as done.Mar 29 2022, 8:07 AM
jhenderson added inline comments.Mar 29 2022, 8:19 AM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
13–15

As this logic is testing generic logic in the tool, rather than format-specific logic, I think you'd be better off pulling it into a separate test directly in the llvm-readobj directory. Additionally, I wouldn't use ELF, as ELF is a good candidate for the next format to support. I'd use one of the other input formats supported by llvm-readobj.

oontvoo added inline comments.Mar 29 2022, 8:28 AM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
13–15

As you've said, this test is temporary until the other formats are implemented. As such, I dont see why it needs to go out in a separate test file

jhenderson added inline comments.Mar 29 2022, 8:30 AM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
13–15

As an ELF developer, I wouldn't necessarily expect me adding support to the ELF layer to break a Mach-O test, which is currently what would happen.

oontvoo marked an inline comment as done.Mar 29 2022, 8:45 AM
oontvoo added inline comments.
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
13–15

k

oontvoo updated this revision to Diff 418893.Mar 29 2022, 8:45 AM
oontvoo marked an inline comment as done.

added more tests

jhenderson added inline comments.Mar 29 2022, 11:45 PM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
231

Nit: missing newline at EOF.

llvm/test/tools/llvm-readobj/sort-symbols.test
2

Double # for comments in these tests, and remove the double space.

7

Any particular reason you've not included xcoff?

17

I'd consider pruning this back to just warning '{{.+}}_macho': to reduce the risk of false negatives due to a slight change in the warning message.

oontvoo marked 4 inline comments as done.Mar 30 2022, 6:24 AM
oontvoo added inline comments.
llvm/test/tools/llvm-readobj/sort-symbols.test
17

Wouldn't that make the test more prone to false positives? that is, if some new warning pops up somewhere else, this would trip. So i'm going to keep this.

oontvoo updated this revision to Diff 419127.Mar 30 2022, 6:24 AM
oontvoo marked an inline comment as done.

updated diff

jhenderson added inline comments.Mar 30 2022, 6:42 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

Yes, it would, but we probably shouldn't be invoking behaviour that causes those warnings anyway, so they're harmless (it would be easy to fix the test if it did trigger one in the future).

The test as it was before the last edit demonstrates why overly strict CHECK-NOTs are not much use, because typos can cause them to pass spuriously.

69

Too many blank lines at EOF (should be exactly one \n at the end).

oontvoo updated this revision to Diff 419129.Mar 30 2022, 6:43 AM

removed blank line

oontvoo marked an inline comment as done.Mar 30 2022, 6:44 AM
oontvoo added inline comments.
llvm/test/tools/llvm-readobj/sort-symbols.test
17

But that's not this test's job to guard against *OTHER* kinds of warnings.

oontvoo marked 2 inline comments as done.Mar 30 2022, 6:44 AM
oontvoo added inline comments.
llvm/test/tools/llvm-readobj/sort-symbols.test
17

Furthermore, false negatives/brittle tests are just as frustrating.

oontvoo added inline comments.Mar 30 2022, 6:51 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

Yes, it would, but we probably shouldn't be invoking behaviour that causes those warnings anyway, so they're harmless (it would be easy to fix the test if it did trigger one in the future).

I disagree with the assertion that it's easy to fix these. Imagine there were a dozen tests similar to this one which were not expecting some warnings, then someone added a new warning and they would have to go update all these tests, even though it's not their fault. (it is the test's fault that it casts too wide a nest on the warning).

oontvoo added inline comments.Mar 30 2022, 6:52 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

s/nest/net

Do you have any other comment on this patch because it seems we've been back on forth for a very long time and it doesn't seem to get any more progress ...

jhenderson added inline comments.Mar 30 2022, 7:10 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

@MaskRay, any thoughts on this or other aspectse of this patch?

As someone who has been stung way too many times by rotten tests caused by negative matches like this not actually catching the thing I'm expecting to be caught, I'm incredibly wary of strict -NOT patterns like this. This isn't some arbitrary concern: I've seen bugs in released products because of this exact kind of overly precise check pattern.

That being said, there is an alternative approach I think you could consider: stick the message after the colon in a FileCheck define and then use it in both the positive and negative matches. That way, if the message is changed, the positive matches will start failing, prompting the developer to update that check too, which in turn will ensure the CHECK-NOT doesn't rot (since it's testing guaranteed to be testing the exact same string).

# RUN: ... | FileCheck %s -DMSG="--sort-symbols is not supported yet for this format"

# CHECK: warning: '{{.+}}_coff': [[MSG]]
...
# CHECK-NOT: warning: '{{.+}}': [[MSG]]

(NB: I've left the file path loose in the negative match so that if somebody changes the input file name, the check pattern is still valid).

oontvoo added inline comments.Mar 30 2022, 7:26 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

As someone who has been stung way too many times by rotten tests caused by negative matches like this not actually catching the thing I'm expecting to be caught, I'm incredibly wary of strict -NOT patterns like this. This isn't some arbitrary concern: I've seen bugs in released products because of this exact kind of overly precise check pattern.

Generally, maybe there's a point - but in this context specifically, what would be the bug if the warning were emitted for macho and not caught? The macho specific tests should have caught it (ie., changes in code logic)

And stated before, you were arguing for making this CHECK-NOT catch all the warnings, which I disagree with (reasons stated a few comments back).
So while it's true that the current set up isn't too ideal, given a choice of false neg vs false positive, I think most would agree that a test should learn toward false positive because false negatives tend to waste a lot of other people's time in debugging test failures.

That being said, there is an alternative approach I think you could consider: stick the message after the colon in a FileCheck define and then use it in both the positive and negative matches. That way, if the message is changed, the positive matches will start failing, prompting the developer to update that check too, which in turn will ensure the CHECK-NOT doesn't rot (since it's testing guaranteed to be testing the exact same string).

# RUN: ... | FileCheck %s -DMSG="--sort-symbols is not supported yet for this format"

# CHECK: warning: '{{.+}}_coff': [[MSG]]
...
# CHECK-NOT: warning: '{{.+}}': [[MSG]]

(NB: I've left the file path loose in the negative match so that if somebody changes the input file name, the check pattern is still valid).

oontvoo added inline comments.Mar 30 2022, 7:34 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

(NB: I've left the file path loose in the negative match so that if somebody changes the input file name, the check pattern is still valid).

What would be the legitimate reason to change the macho filename and not update the test? Those names were chosen specifically to differentiate the different formats.

oontvoo updated this revision to Diff 419137.Mar 30 2022, 7:37 AM

pass MSG param to FileCheck

oontvoo updated this revision to Diff 419215.Mar 30 2022, 10:49 AM

updated diff

MaskRay added inline comments.Mar 30 2022, 10:59 AM
llvm/test/tools/llvm-readobj/MachO/stabs-sorted.yaml
2
llvm/test/tools/llvm-readobj/sort-symbols.test
17

The current # CHECK-NOT: warning '{{.+}}_macho': [[MSG]] usage looks quite nice, though you may omit : after warning. By saving MSG to a -D variable, you don't have to repeat the message.

I also wonder whether we can just use # CHECK-NOT: warning: to assert no extra warnings. To make the test more specific, we want to make the file as valid as possible and rule out possibilities for other warnings, then # CHECK-NOT: warning: suffices.

oontvoo updated this revision to Diff 419220.Mar 30 2022, 11:07 AM
oontvoo marked 2 inline comments as done.

updated diff

oontvoo added inline comments.Mar 30 2022, 11:11 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

though you may omit : after warning. By saving MSG to a -D variable, you don't have to repeat the message.

I'm not sure factoring the : to MSG helps with readability - the CHECK statements would look like:

# CHECK: warning '{{.+}}_coff'[[MSG]]

which is not as good as the current form

oontvoo added inline comments.Mar 30 2022, 11:25 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

I also wonder whether we can just use # CHECK-NOT: warning: to assert no extra warnings. To make the test more specific, we want to make the file as valid as possible and rule out possibilities for other warnings, then # CHECK-NOT: warning: suffices.

Why does only this test need to assert that there is no other warnings/errors? I've checked all other tests in this project, and none of them has a check that no additional warnings were emitted.

oontvoo added inline comments.Mar 30 2022, 11:38 AM
llvm/test/tools/llvm-readobj/sort-symbols.test
17

IOWs, I don't see why the burden has to be on this test to check that there is no other warnings. It should be as precise as possible in terms of what warning it does not expect and in this case, it does not expect this specific warning for the macho format. Any other warnings is another test's problem. (This test is named "sort-symbols.test" and not "all-warnings.test" for a reason)

If we want to have warning check tests, that's another discussion.

jhenderson accepted this revision.Mar 30 2022, 11:58 PM

LGTM now, thanks!

This revision is now accepted and ready to land.Mar 30 2022, 11:58 PM
This revision was landed with ongoing or failed builds.Mar 31 2022, 6:16 AM
This revision was automatically updated to reflect the committed changes.