[DWARF] Try not to crash for codes with missing debug information

[jdoerfert]

Summary

This prevents crashes in the OpenMP offload pipeline as not everything
is properly annotated with debug information, e.g., the runtimes we link
in. While we might want to have them annotated, it seems to be generally
useful to gracefully handle missing debug info rather than crashing.

TODO: A test is missing and can hopefully be distilled prior to landing.

This fixes #51079.

[ye-luo]

Much appreciated. Allowing to debug the host code is already a huge plus.

[arsenm]

The test is the most important part of this patch

[dblaikie]

Could you add a test for this?

& I'm confused by why this change would be required for an OpenMP use case, since we've supported the nodebug attribute in Clang for a while, which omits the debug info from a subprogram - which would've tickled the same codepath, I'd havze thought. So I guess there's something more interesting about this situation than "a function without debug info" - the test case would help illustrate this & may be worth discussing once it's provided.

[jdoerfert]

I plan to distill a test from the bug report before I move this further. Wanted to put it here to get feedback and not forget about the patch file in my local folder ;)

[jdoerfert]

Finally reduced the reproducer: https://godbolt.org/z/dzKf9devd

[dblaikie]

Ah, so the entry point to the problematic caller (emitInitialRawDwarfLocDirective) is NVPTX-specific, it seems. And hadn't been tested with a Module that contained a DICompileUnit and also contained at least one Function without a DISubprogram attachment? OK. Will be good to have that test case added (I guess it probably doesn't need any instructions in the function, unless that's the minimum necessary to have a valid NVPTX function?)

I would wonder why NVPTX has this unique codepath, but this probably isn't the place to relitigate that design decision (though maybe some comments in emitInitialRawDwarfLocDirective would be helpful to explain the circumstances).

I /think/ it'd be more suitable for the subprogram check to be a layer up, in the emitInitialRawDwarfLocDirective function - similar to the other, more mainstream/used by other targets, call to emitInitialLocDirective that only call it when the function has a subprogram attached.

[tra]

In D116959#3255312, @dblaikie wrote:

I would wonder why NVPTX has this unique codepath

PTX has rather odd assembly which places restrictions on where it accepts various directives. @ABataev would probably be the best person to ask for the details.

[ABataev]

In D116959#3255401, @tra wrote:

In D116959#3255312, @dblaikie wrote:

I would wonder why NVPTX has this unique codepath

PTX has rather odd assembly which places restrictions on where it accepts various directives. @ABataev would probably be the best person to ask for the details.

You’re right. I don’t remember the details already but most probably it is caused by the NVPTX format and its limitations.

[jdoerfert]

Address comments, add test

[jdoerfert]

From the earlier comments I'm assuming this is good to go. I'll commit stuff later and intend to include this if I don't hear anything to the contrary

[dblaikie]

In D116959#3255604, @jdoerfert wrote:

From the earlier comments I'm assuming this is good to go. I'll commit stuff later and intend to include this if I don't hear anything to the contrary

Yep, sounds good to me. The DebugLoc on the load instruction is necessary, I take it? (I guess it could go on a ret instruction and have the @test function be void()?) That might point to another solution/direction - something is checking for debug locations, and maybe that something should be the one checking for DISubprogram on the function... *looks through the code* I don't immediately see where the existence of a !dbg on an instruction makes a difference here. (but simplified down the original godbolt to this: https://godbolt.org/z/dzGhPP51x and definitely removing the !dbg attachment for the instruction avoids the bug... )

[jdoerfert]

In D116959#3255632, @dblaikie wrote:

In D116959#3255604, @jdoerfert wrote:

From the earlier comments I'm assuming this is good to go. I'll commit stuff later and intend to include this if I don't hear anything to the contrary

Yep, sounds good to me. The DebugLoc on the load instruction is necessary, I take it? (I guess it could go on a ret instruction and have the @test function be void()?) That might point to another solution/direction - something is checking for debug locations, and maybe that something should be the one checking for DISubprogram on the function... *looks through the code* I don't immediately see where the existence of a !dbg on an instruction makes a difference here. (but simplified down the original godbolt to this: https://godbolt.org/z/dzGhPP51x and definitely removing the !dbg attachment for the instruction avoids the bug... )

I'll make it a single ret instruction with !dbg, that "works" (=crashes).

[dblaikie]

I did eventually figure out why the !dbg is important - findPrologueEndLoc returns a null DebugLoc if there are no !dbg attachments, so avoids the DISubprogram dereference that way. So, yeah, at least now I understand why this only happens in a non-debug-having function that has debug locations in it, only under NVPTX.

Thanks for your patience :)

[jdoerfert]

Add run line

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

FileNo = static_cast<DwarfCompileUnit &>(*DCUs[CUID])
                  .getOrCreateSourceID(Scope->getFile());
   }
   Asm.OutStreamer->emitDwarfLocDirective(FileNo, Line, Col, Flags, 0,
                                          Discriminator, Fn);
 }
 
 DebugLoc DwarfDebug::emitInitialLocDirective(const MachineFunction &MF,
                                              unsigned CUID) {
+  // If we have code without debug information we need to give up.
+  DISubprogram *MFSP = MF.getFunction().getSubprogram();
+  if (!MFSP)
+    return DebugLoc();
+
   // Get beginning of function.
   if (DebugLoc PrologEndLoc = findPrologueEndLoc(&MF)) {
     // Ensure the compile unit is created if the function is called before
     // beginFunction().
-    (void)getOrCreateDwarfCompileUnit(
-        MF.getFunction().getSubprogram()->getUnit());
+    (void)getOrCreateDwarfCompileUnit(MFSP->getUnit());
     // We'd like to list the prologue as "not statements" but GDB behaves
     // poorly if we do that. Revisit this with caution/GDB (7.5+) testing.
     const DISubprogram *SP = PrologEndLoc->getInlinedAtScope()->getSubprogram();
     ::recordSourceLine(*Asm, SP->getScopeLine(), 0, SP, DWARF2_FLAG_IS_STMT,
                        CUID, getDwarfVersion(), getUnits());
     return PrologEndLoc;
   }
   return DebugLoc();