Page MenuHomePhabricator

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

Authored by jdoerfert on Jan 10 2022, 10:35 AM.

Details

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.

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 10 2022, 10:35 AM
jdoerfert requested review of this revision.Jan 10 2022, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2022, 10:35 AM
Herald added subscribers: sstefan1, wdng. · View Herald Transcript
jdoerfert edited the summary of this revision. (Show Details)Jan 10 2022, 10:36 AM
ye-luo accepted this revision.Jan 10 2022, 10:40 AM

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

This revision is now accepted and ready to land.Jan 10 2022, 10:40 AM

The test is the most important part of this patch

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 planned changes to this revision.Jan 10 2022, 11:15 AM

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

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 added a subscriber: ABataev.Jan 19 2022, 10:02 AM

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.

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.

Address comments, add test

This revision is now accepted and ready to land.Jan 19 2022, 10:39 AM

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 accepted this revision.Jan 19 2022, 10:51 AM
dblaikie added a subscriber: test.

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... )

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).

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 updated this revision to Diff 401408.Jan 19 2022, 2:34 PM

Add run line