Warn when running a pass not compatible with DWARF #3506

kripken · 2021-01-21T23:01:24Z

Previously the addDefault* methods would avoid adding opt passes that we
know are incompatible with DWARF. However, that didn't handle the case of
passes that are added in other ways. For example, when running Asyncify,
emcc will run --flatten before, and that pass is not compatible with DWARF.
This PR lets us warn on that by annotating the passes themselves. Then we
use those annotation to either not run a pass at all (matching the previous
behavior) or to show a warning when necessary.

Open to better ideas for the addIfDWARFAllowed name - ?

Fixes emscripten-core/emscripten#13288 . That is, concretely
after this PR running asyncify + DWARF will show a warning to the user.

tlively

Looks good to me. Maybe addIfDWARFPreserved instead of addIfDWARFAllowed?

tlively · 2021-01-21T23:26:38Z

test/unit/test_dwarf.py

@@ -14,3 +15,14 @@ def test_no_crash(self):
            args = [os.path.join(path, name)] + \
                   ['-g', '--dwarfdump', '--roundtrip', '--dwarfdump']
            shared.run_process(shared.WASM_OPT + args, capture_output=True)
+
+    def test_dwarf_incompatibility(self):


This could make a really simple lit test:

;; RUN: wasm-opt -g <path to cubescript.wasm> --flatten 2>&1 | filecheck %s --check-prefix WARN ;; RUN: wasm-opt -g <path to cubescript.wasm> --metrics 2>&1 | filecheck %s --check-prefix OK ;; WARN: not fully compatible with DWARF ;; OK-NOT: not fully compatible with DWARF

Ok, happy to try to join the lit bandwagon...

Do we have instructions anywhere for how to set things up locally? ./check.py lit gives me errors,

parse-error.wast.script: line 1: filecheck: command not found

I have done pip install filecheck (that worked for lit in the past iirc) but that appears to not be enough. Do I need a build of LLVM or something like that?

pip3 install -r dev-requirements.txt should get you up and running. Thanks to @sbc100 for suggesting that we use such a file.

(looks like it's requirements-dev.txt?)

Ok, I ran pip3 install -r requirements-dev.txt and it succeeded, but I still get filecheck: command not found.

I am not very familiar with pip. It did not install anything called *filecheck* under my current dir. Does it install to a global location?

Pip install (when run without --system) install things into a user-local PATH. On linux (at least on my machine) this is $HOME/.local/bin. I think the expectation is that this would be in your PATH, or that you would add it to your path. I'm having trouble finding docs on best practices around how to ensure its added.

Thanks!

Indeed that location is not in my path. Adding it fixes things...

Ok, looks like I need to add it as a .wast file, even though it's not a wast. Now it passes.

However, I am a little unsure this is better. It has

;; WARN: not fully compatible with DWARF ;; OK-NOT: not fully compatible with DWARF

The first line is robust in that a typo will make the test fail. The second line is brittle, however. In the python version a single string is used to avoid that risk, but it must be duplicated in the lit...?

If that's true then I think I'd prefer to keep it as python. But I don't feel strongly?

Yes, the line has to be duplicated in the lit test, but on the whole I still prefer the lit test over the Python test due to the relative verbosity of the latter. If you want to use a different file extension, you can add it to the lit config here: https://github.com/WebAssembly/binaryen/blob/main/test/lit/lit.cfg.py#L6.

Ok, I don't feel strongly so let's make it lit. Pushed now, PTAL.

kripken · 2021-01-21T23:48:11Z

addIfDWARFPreserved

Hmm, but it's "if DWARF must be preserved, or the pass doesn't mess with DWARF"...

maybe addIfDWARFCompatible?

tlively · 2021-01-22T00:58:54Z

The tricky thing about the name is that you're not really selecting on a property of the pass itself, but rather a property of the current configuration combined with the pass. DWARFCompatible sounds too much to me like it's saying the added pass is compatible with DWARF, but it might not be in general! My thought process behind DWARFPreserved is that it says something about the DWARF (which might not exist and therefore be trivially preserved) rather than about the added pass.

That's all kind of pedantic, though, so whatever sounds best to you is fine with me :)

aheejin · 2021-01-22T10:44:09Z

src/passes/pass.cpp

+  auto pass = PassRegistry::get()->createPass(passName);
+  if (!pass->invalidatesDWARF() ||
+      !Debug::shouldPreserveDWARF(options, *wasm)) {
+    doAdd(std::move(pass));


When the pass does not support DWARF, In which case we don't add the pass and in which case we just warn? doAdd seems to just warn, but this function, which calls doAdd, seems not call doAdd in the first place.

doAdd always adds. It may warn as needed.

This method addIfDWARFAllowed should be something like addPassIfNoDWARFWarningWouldBeShown. That is, this method is called when the addition is optional, and in that case it's best to not add the pass if there are issues.

kripken · 2021-01-22T18:22:23Z

I renamed to addIfNoDWARFIssues and improved the comment - is that better?

This reverts commit 785f57c.

kripken · 2021-01-26T01:03:55Z

@tlively

I had to revert the lit test and return to the old one. For some reason with lit it fails like this: https://github.com/WebAssembly/binaryen/runs/1764105570 - it just stops, without a clear error. It does not reproduce locally. I spent some time on this, tested some theories like maybe the new warning increases our logs to a dangerous size, but it's not that - and the tests now pass again without the lit test. Maybe the lit tests use more memory because they run in parallel or some other resource exhaustion? (Not urgent for this PR, I don't think, but I can file an issue for it unless this is something obvious that you recognize.)

tlively · 2021-01-26T06:45:46Z

I suspect that the working directory for the test on the bots is different than in your local setup, so the relative path the test uses works for you but not for the bots. The working directory for the lit tests is configured here to be the test directory under the build directory. You probably have an in-tree build, but the bot has an out-of-tree build in which the build directory does not contain the cubescript.wasm file.

It's probably not worth reconfiguring the lit tests or duplicating the cubescript.wasm input file (assuming it's used by other tests), so keeping this as a python test lgtm.

kripken · 2021-01-26T14:49:16Z

@tlively I see, thanks - yes, that could make sense.

Any idea why it doesn't show a proper error though? That's really confusing...

tlively · 2021-01-26T18:23:33Z

The error looks fine from what I can see. From the logs:

2021-01-25T18:39:04.8914362Z ******************** TEST 'Binaryen lit tests :: dwarf.wast' FAILED ********************
2021-01-25T18:39:04.8915476Z Script:
2021-01-25T18:39:04.8916241Z --
2021-01-25T18:39:04.8917989Z : 'RUN: at line 2';   /home/runner/work/binaryen/binaryen/out/bin/wasm-opt -g unit/input/dwarf/cubescript.wasm --flatten 2>&1 | filecheck /home/runner/work/binaryen/binaryen/test/lit/dwarf.wast --check-prefix WARN
2021-01-25T18:39:04.8920792Z : 'RUN: at line 6';   /home/runner/work/binaryen/binaryen/out/bin/wasm-opt -g unit/input/dwarf/cubescript.wasm --metrics 2>&1 | filecheck /home/runner/work/binaryen/binaryen/test/lit/dwarf.wast --check-prefix OK
2021-01-25T18:39:04.8923230Z --
2021-01-25T18:39:04.8923892Z Exit Code: 1
2021-01-25T18:39:04.8924347Z 
2021-01-25T18:39:04.8924956Z Command Output (stdout):
2021-01-25T18:39:04.8925730Z --
2021-01-25T18:39:04.8926723Z /home/runner/work/binaryen/binaryen/test/lit/dwarf.wast:3:10: error: WARN: expected string not found in input
2021-01-25T18:39:04.8927758Z ;; WARN: not fully compatible with DWARF
2021-01-25T18:39:04.8928425Z          ^
2021-01-25T18:39:04.8929050Z <stdin>:1:1: note: scanning from here
2021-01-25T18:39:04.8930136Z Failed opening 'unit/input/dwarf/cubescript.wasm'
2021-01-25T18:39:04.8931087Z ^
2021-01-25T18:39:04.8931986Z <stdin>:1:25: note: possible intended match here
2021-01-25T18:39:04.8933130Z Failed opening 'unit/input/dwarf/cubescript.wasm'
2021-01-25T18:39:04.8933948Z                         ^
2021-01-25T18:39:04.8934381Z 
2021-01-25T18:39:04.8935773Z --
2021-01-25T18:39:04.8936401Z Command Output (stderr):
2021-01-25T18:39:04.8937155Z --
2021-01-25T18:39:04.8937834Z + : 'RUN: at line 2'
2021-01-25T18:39:04.8938933Z + filecheck /home/runner/work/binaryen/binaryen/test/lit/dwarf.wast --check-prefix WARN
2021-01-25T18:39:04.8940424Z + /home/runner/work/binaryen/binaryen/out/bin/wasm-opt -g unit/input/dwarf/cubescript.wasm --flatten
2021-01-25T18:39:04.8941355Z 
2021-01-25T18:39:04.8942086Z --
2021-01-25T18:39:04.8942445Z 
2021-01-25T18:39:04.8942842Z ********************

kripken · 2021-01-26T19:02:05Z

Oh, sorry @tlively , I was reading the log incorrectly...

kripken added 9 commits January 20, 2021 11:03

wip

b0ae3ed

format

1d690a3

more

6e62f8e

format

7d37976

more

2f98d08

fix

08e2a1c

better

f3abc42

fixes emscripten-core/emscripten#13288

67b8450

fix

40f90c1

kripken requested review from tlively and aheejin January 21, 2021 23:01

fix some compiler warnings on clang

b1b5a97

tlively approved these changes Jan 21, 2021

View reviewed changes

fix emscripten build

22e6ce2

format

ca8b5e0

aheejin reviewed Jan 22, 2021

View reviewed changes

rename

e207e80

aheejin approved these changes Jan 23, 2021

View reviewed changes

kripken added 2 commits January 25, 2021 10:13

format

07f3b16

switch test

785f57c

tlively approved these changes Jan 25, 2021

View reviewed changes

kripken added 6 commits January 25, 2021 10:28

flake8

00aa84b

improve logging to try to figure out why CI hates this PR

019fcb5

format

80a7997

Revert "switch test"

85e0dd4

This reverts commit 785f57c.

restore

c021f5c

revert

b9d7d58

tlively approved these changes Jan 26, 2021

View reviewed changes

pfaffe mentioned this pull request Jan 26, 2021

Support DWP files emscripten-core/emscripten#13251

Closed

kripken merged commit 89164cd into main Jan 26, 2021

kripken deleted the warn branch January 26, 2021 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when running a pass not compatible with DWARF #3506

Warn when running a pass not compatible with DWARF #3506

kripken commented Jan 21, 2021

tlively left a comment

tlively Jan 21, 2021

kripken Jan 21, 2021

tlively Jan 22, 2021

kripken Jan 22, 2021

sbc100 Jan 22, 2021

kripken Jan 22, 2021

kripken Jan 22, 2021

tlively Jan 22, 2021

kripken Jan 25, 2021

kripken commented Jan 21, 2021

tlively commented Jan 22, 2021

aheejin Jan 22, 2021

kripken Jan 22, 2021

kripken commented Jan 22, 2021

kripken commented Jan 26, 2021

tlively commented Jan 26, 2021

kripken commented Jan 26, 2021

tlively commented Jan 26, 2021

kripken commented Jan 26, 2021

Warn when running a pass not compatible with DWARF #3506

Warn when running a pass not compatible with DWARF #3506

Conversation

kripken commented Jan 21, 2021

tlively left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jan 21, 2021

tlively commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jan 22, 2021

kripken commented Jan 26, 2021

tlively commented Jan 26, 2021

kripken commented Jan 26, 2021

tlively commented Jan 26, 2021

kripken commented Jan 26, 2021