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

Vy Nguyen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 30 11:38:20 PDT 2022


oontvoo added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/sort-symbols.test:16
+# CHECK: Format: aixcoff-rs6000
+# CHECK-NOT: warning '{{.+}}_macho': --sort-symbols is not supported yet for this format.
+# CHECK: Format: Mach-O 64-bit x86-64
----------------
oontvoo wrote:
> oontvoo wrote:
> > MaskRay wrote:
> > > oontvoo wrote:
> > > > oontvoo wrote:
> > > > > jhenderson wrote:
> > > > > > oontvoo wrote:
> > > > > > > oontvoo wrote:
> > > > > > > > oontvoo wrote:
> > > > > > > > > oontvoo wrote:
> > > > > > > > > > jhenderson wrote:
> > > > > > > > > > > oontvoo wrote:
> > > > > > > > > > > > jhenderson wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > 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.
> > > > > > > > > > > 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.
> > > > > > > > > > But that's not this test's job to guard against *OTHER* kinds of warnings.
> > > > > > > > > Furthermore, false negatives/brittle tests are just as frustrating.
> > > > > > > > > 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).
> > > > > > > > 
> > > > > > > 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 ...
> > > > > > @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).
> > > > > 
> > > > > > 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).
> > > > > 
> > > > > 
> > > > 
> > > > > (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.   
> > > 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.
> > > 
> > >  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
> > 
> 
> > 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. 
> 
> 
> 
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. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116787



More information about the llvm-commits mailing list