Page MenuHomePhabricator

[WebAssembly] Fixed FrameBaseLocal not being set.
ClosedPublic

Authored by aardappel on Mar 5 2020, 4:34 PM.

Details

Summary

Fixes: https://bugs.llvm.org/show_bug.cgi?id=44920

WebAssemblyRegColoring may merge the vreg that currently represents
the FrameBase with one representing an argument.
WebAssemblyExplicitLocals picks up the corresponding local when
a vreg is first added to the Reg2Local mapping, except when it is
an argument instruction which are handled separately.

Note that this does not change that vregs representing the FrameBase
may get merged, it is not clear to me that this may have other
effects we may want to avoid?

Diff Detail

Event Timeline

aardappel created this revision.Mar 5 2020, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 4:34 PM

Yeah, this definitely looks like the likely fix. It's the spot where a local gets assigned that doesn't go through getLocalId. It seems to make sense to still allow this merging, as it would reduce the total number of locals. I guess it does mean that the frame base info would be incorrect when that register has the argument's value rather than the frame base's value, but presumably that would be ok, since the frame base should be live anytime a variable on the stack is live.

Were you able to reduce the IR that resulted in this to something small enough we could put in a test?

I reduced the C++ code that generates this, not any LLVM IR, but its still a fair amount of code since it pulls in a bunch of headers. Quickest would probably to generate the IR and then construct a new test based on that, if you think its useful.

It's probably worth an attempt, and that sounds like the right approach. A crash bug like this is often not too hard to reduce with bugpoint; i.e. you'd start with clang using -emit-llvm to get the IR, and then let bugpoint reduce the IR.

aardappel updated this revision to Diff 248635.Mar 5 2020, 5:37 PM

clang-format

The original .ll is 145K, which bugpoint then reduces 119K. Bugpoint is actually able to reduce most function bodies to nothing, but it leaves in all the debug information, and any attempt at reducing it either errors or makes the repro go away. Also, bugpoint makes some identifiers really long (in an attempt to encode its search space?) that would all have to be renamed. I don't see an easy way to make this an acceptable looking test case that we could add as an llvm-lit test.

Hm, that's too bad. In theory it seems like it ought to be possible to just remove all the debug info from the IR that's attached to functions (i.e. all the debug.declare/debug.values) and the debug metadata that's associated with all functions other than this one, since this is a completely function-local transform that really only depends on there being any CU information at all.
I've also sometimes had good luck with c-reduce to try to reduce it at the C level rather than the IR level. It might be worth a try.
Or maybe we've let this go long enough, and we could just say that we'll have coverage if we do any continuous builds of a reasonable test suite with debug info?

Even removing the debug decls at the end of the file still leaves a 37K very messy file that I would not want to add as a test :)

I am probably biased, but I don't think this is worth further effort. This is a small issue that very easily have been been written correctly initially (like so many bits of correct code for which we don't have explicit tests), it just wasn't.

I'm sure further coverage of debug functionality is yet to come.

dschuff accepted this revision.Mar 9 2020, 4:47 PM

Yeah, let's just go ahead and land this. I think once this rolls we can enable DWARF with -g by default and start to get more feedback.

This revision is now accepted and ready to land.Mar 9 2020, 4:47 PM
This revision was automatically updated to reflect the committed changes.