Page MenuHomePhabricator

[lld-macho] Error out if we encounter a HelpHidden flag in tests
AbandonedPublic

Authored by int3 on Dec 1 2020, 8:31 PM.

Details

Reviewers
thakis
compnerd
Group Reviewers
Restricted Project
Summary

@thakis caught a bunch of implemented flags for which HelpHidden wasn't
removed. This diff ensures that we prevent those issues going ahead.

I initially considered making %lld expand to include
-fatal_warnings, but that would've made testing for desired warnings
more awkward. Hence this approach.

Diff Detail

Event Timeline

int3 created this revision.Dec 1 2020, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 8:31 PM
int3 requested review of this revision.Dec 1 2020, 8:31 PM
compnerd accepted this revision.Dec 2 2020, 8:46 AM
compnerd added a subscriber: compnerd.

I still don't really like the environment variable approach to the testing.

This revision is now accepted and ready to land.Dec 2 2020, 8:46 AM

I'm also not a super fan of things depending on env vars:

  1. I often copy-paste the output of llvm-lit lld/tests/some-test.s -vv. LLD_IN_TEST isn't in that output but set implicitly by lit, so that means copying the env now has magically different behavior (for that reason, a whole bunch of tests in the COFF port explicitly set env LLD_IN_TESTS=1 redundantly -- but we'd have to add that to every single test)
  1. This gives LLD_IN_TEST a much wider semantic meaning than it had before. Before, it's a very targeted thing for making sure we do fast exit and global exception handling and test repeats; now it affects basically anything

I think globally passing -fatal_warnings is a bit less surprising, and it's not so bad. I prootyped it in D92575 -- wdyt? (I can put in the two test.s changes you have in here if you like it.)

int3 abandoned this revision.Dec 3 2020, 3:16 PM

Good points! I do copy-paste llvm-lit's output pretty often too...

Let's go with yours :)