Page MenuHomePhabricator

[WebAssembly] Fixed debugloc in DebugFixup pass
ClosedPublic

Authored by aardappel on May 15 2020, 10:12 AM.

Details

Summary

BuildMI requires this debug loc to be from the same sub program as the variable metadata passed in.

Diff Detail

Event Timeline

aardappel created this revision.May 15 2020, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2020, 10:12 AM
dschuff accepted this revision.May 15 2020, 10:15 AM
This revision is now accepted and ready to land.May 15 2020, 10:15 AM

I guess the existing tests didnt catch this because the pop is still within the same subprogram. Did you see this just at the end of a function, or where?

@dschuff not sure what the actual code looks like, this is part of the Bullet3 tests that failed.

Added differential revision to commit

This revision was automatically updated to reflect the committed changes.
aheejin added inline comments.May 15 2020, 1:40 PM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
121

clang-format. You can use https://reviews.llvm.org/project/view/78/ to detect this kind of issues. (This emits a lot of errors for other people's code, so it's a bit annoying.)

aardappel marked 2 inline comments as done.May 15 2020, 5:46 PM
aardappel added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
121

Thanks, joined that group. I did git clang-format this CL though.

aheejin added inline comments.May 15 2020, 8:08 PM
llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
121

Locally running clang-format shows diff of this:

--- a/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp
@@ -118,8 +118,9 @@ bool WebAssemblyDebugFixup::runOnMachineFunction(MachineFunction &MF) {
               // a $noreg DBG_VALUE for the variable to end it, right after
               // the current instruction.
               BuildMI(*Prev.DebugValue->getParent(), std::next(MII),
-                      Prev.DebugValue->getDebugLoc(), TII->get(WebAssembly::DBG_VALUE), false,
-                      Register(), Prev.DebugValue->getOperand(2).getMetadata(),
+                      Prev.DebugValue->getDebugLoc(),
+                      TII->get(WebAssembly::DBG_VALUE), false, Register(),
+                      Prev.DebugValue->getOperand(2).getMetadata(),
                       Prev.DebugValue->getOperand(3).getMetadata());
             }
           }

If there's a difference between wouter's clang-format output and heejin's / the bots, it could be that the default clang-format on debian is really old. that's an issue I've seen before.

aardappel marked an inline comment as done.May 21 2020, 4:36 PM

Yes, likely.
Either way, I generally don't think such a small change needs a follow-up commit.

aheejin added a comment.EditedMay 21 2020, 5:02 PM

Yes, likely.
Either way, I generally don't think such a small change needs a follow-up commit.

  • As I said before, if you think something is unnecessary, I'd appreciate if you just say so, rather than ignoring the comment itself. (I feel I saw this several times even before I suggested this to you first in the previous CL.) I think this issue is minor too, but I pinged you because of that.
  • Many people fix minor things or follow-ups with just commits without uploading a CL in Phabricator when they don't really need reviews. Not suggesting to fix exactly this, but just FYI for the next time.