[WebAssembly] Fixed debugloc in DebugFixup pass

[aardappel]

Summary

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

[dschuff]

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?

[aardappel]

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

[aardappel]

Added differential revision to commit

[aheejin]

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]

llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp

121

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

[aheejin]

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());
             }
           }

[aheejin]

Ping

[dschuff]

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]

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

[aheejin]

In D80019#2050293, @aardappel wrote:

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.

llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp

for (auto MII = MBB.begin(); MII != MBB.end(); ++MII) {
             assert(Prev.Reg == MO.getReg() &&
                    "WebAssemblyDebugFixup: Pop: Register not matched!");
             if (Prev.DebugValue) {
               // This stackified reg is a variable that started life at
               // Prev.DebugValue, so now that we're popping it we must insert
               // a $noreg DBG_VALUE for the variable to end it, right after
               // the current instruction.
               BuildMI(*Prev.DebugValue->getParent(), std::next(MII),
-                      MI.getDebugLoc(), TII->get(WebAssembly::DBG_VALUE), false,
+                      Prev.DebugValue->getDebugLoc(), TII->get(WebAssembly::DBG_VALUE), false,
                       Register(), Prev.DebugValue->getOperand(2).getMetadata(),
                       Prev.DebugValue->getOperand(3).getMetadata());
             }
           }
         }
         for (MachineOperand &MO : MI.defs()) {
           if (MO.isReg() && MFI.isVRegStackified(MO.getReg())) {
             Stack.push_back({MO.getReg(), nullptr});