[DebugInfo] Include DW_TAG_skeleton_unit when looking for parent UnitDie

[wenlei]

Summary

DIE::getUnitDie looks up parent DIE until compile unit or type unit is found. However for skeleton CU with debug fission, we would have DW_TAG_skeleton_unit instead of DW_TAG_compile_unit as top level DIE.

This change fixes the look up so we can get DW_TAG_skeleton_unit as UnitDie for skeleton CU.

[dblaikie]

This should include a test - and could you describe more about where this issue came up/what motivated the change?

[wenlei]

Add test case.

[wenlei]

In D120610#3348997, @dblaikie wrote:

This should include a test - and could you describe more about where this issue came up/what motivated the change?

Sure, sorry for the delay as I just got back to this today. I sent the fix first hoping to get a sanity check as we were a bit puzzled how come nobody hit this in the past.

Here's what happened for the ICE - we needed to add template types when constructing subprogram die for template functions (addTemplateParams) for the skeleton CU, this is done through getOrCreateContextDIE which involves finding/creating parent DIEs. For that, we sometimes need to retrieve top level DIE (getOrCreateContextDIE -> getUnitDie). Before this fix, we end up with nullptr from getUnitDie, so some DIEs created in that process has getUnit() being null which leads to ICE on access.

I added a test case that would cause ICE without this fix (the set of flags there is minimum).

[dblaikie]

Fixes in LLVM require tests in LLVM - probably taking the clang test and compiling that to llvm IR (include the original C++ source in a comment in the IR test case) and then testing it in LLVM instead of clang.

Also looks like the test could be simplified a bit more:

void f1();

template <typename T>
void f2() {
  f1();
}

void f3() {
  f2<int>();
}

[wenlei]

update test case

[wenlei]

In D120610#3373042, @dblaikie wrote:

Fixes in LLVM require tests in LLVM - probably taking the clang test and compiling that to llvm IR (include the original C++ source in a comment in the IR test case) and then testing it in LLVM instead of clang.

Also looks like the test could be simplified a bit more:

void f1();

template <typename T>
void f2() {
  f1();
}

void f3() {
  f2<int>();
}

Done, thanks.

[dblaikie]

It'd be good to include some testing beyond "does not crash" - like what was the specific debug info we were trying to create when the crash was hit? Perhaps we should be testing that (since the crash demonstrates we weren't testing that anywhere else)

[wenlei]

Add test check for template type parameters

[wenlei]

In D120610#3376002, @dblaikie wrote:

It'd be good to include some testing beyond "does not crash" - like what was the specific debug info we were trying to create when the crash was hit? Perhaps we should be testing that (since the crash demonstrates we weren't testing that anywhere else)

Sure, added a few checks around DW_TAG_template_type_parameter for both DW_TAG_skeleton_unit and DW_TAG_skeleton_unit. Let me know whether what I have here aligns with the convention of similar tests.

[dblaikie]

Thanks, sounds good!

[ayermolo]

@wenlei Thanks for digging in to this!

llvm/lib/CodeGen/AsmPrinter/DIE.cpp

uint64_t DIE::getDebugSectionOffset() const {
   assert(Unit && "DIE must be owned by a DIEUnit to get its absolute offset");
   return Unit->getDebugSectionOffset() + getOffset();
 }
 
 const DIE *DIE::getUnitDie() const {
   const DIE *p = this;
   while (p) {
     if (p->getTag() == dwarf::DW_TAG_compile_unit ||
+        p->getTag() == dwarf::DW_TAG_skeleton_unit ||
         p->getTag() == dwarf::DW_TAG_type_unit)
       return p;
     p = p->getParent();
   }
   return nullptr;
 }
 
 DIEUnit *DIE::getUnit() const {