Page MenuHomePhabricator

Fix faulty assertion for void type in debug info
ClosedPublic

Authored by amccarth on Dec 14 2017, 4:54 PM.

Details

Summary

It appears the code uses nullptr to represent a void type in debug metadata, which led to an assertion failure when building DeltaAlgorithm.cpp with a self-hosted clang on Windows.

The problem occurs when selecting both Dwarf and CodeView. The CodeView causes some additional DI records (for nested typedefs) to be emitted. The Dwarf handler wasn't prepared for these records when they reference void type (which is represented internally by a null pointer).

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=35543

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth created this revision.Dec 14 2017, 4:54 PM

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp
167 ↗(On Diff #127048)

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

I'm not happy with it either. I put this up hoping to get some tips. I have a .cpp test (see the linked bug report). I'll reduce it to an IR test.

amccarth updated this revision to Diff 127200.Dec 15 2017, 3:36 PM

Made part of the fix more localized, preserving one of the assertions.

Added an IR test that demonstrates the problem.

rnk succinctly described the problem in the bug report:

Right, this bug only occurs if the user asks for *both* DWARF and CodeView. We don't really support that configuration, but we didn't take any steps to disable it because there's nothing technically challenging about allowing it.

In this case, what's happening is we're emitting extra DI metadata (nested typedefs, in this case the one for 'void') to support CodeView, and the DWARF asm printer doesn't seem ready for it.

amccarth edited the summary of this revision. (Show Details)Dec 18 2017, 2:00 PM
amccarth added a reviewer: aprantl.
aprantl accepted this revision.Dec 19 2017, 8:49 AM

Looks like CGDebugInfo::CreateType creates void as nullptr. If me modify the testcase to use the typedef

class A {
  typedef void _Nodeptr;
  _Nodeptr * n;
};

we can force it to be emitted even in DWARF:

0x00000068:     DW_TAG_typedef
                  DW_AT_name	("_Nodeptr")
                  DW_AT_decl_file	("/tmp/t.cpp")
                  DW_AT_decl_line	(2)

0x0000006f:     NULL

this also looks weird, but is apparently sort of legal DWARF:

For example, in C and C++ the language implementation can provide an unspecified type entry with the name “void” which can be referenced by the type attribute of pointer types and typedef declarations for ’void’.

although it would be nicer to use DW_TAG_unspecified_type instead.

Based on this it looks like we should allow this case.

llvm/test/DebugInfo/void-typedef.ll
34 ↗(On Diff #127200)

We may not even need the function for the testcase to work, you might get away with removing the function, adding the type to the retained types in the CU and then running the file through llvm-as - | llvm-dis to get rid of all unreachable MDNodes.

This revision is now accepted and ready to land.Dec 19 2017, 8:49 AM
This revision was automatically updated to reflect the committed changes.
0x00000068:     DW_TAG_typedef
                  DW_AT_name	("_Nodeptr")
                  DW_AT_decl_file	("/tmp/t.cpp")
                  DW_AT_decl_line	(2)

0x0000006f:     NULL

this also looks weird, but is apparently sort of legal DWARF:

For example, in C and C++ the language implementation can provide an unspecified type entry with the name “void” which can be referenced by the type attribute of pointer types and typedef declarations for ’void’.

although it would be nicer to use DW_TAG_unspecified_type instead.

Based on this it looks like we should allow this case.

I didn't think it was legal either, but the description of DW_TAG_typedef does say it "may" have a DW_AT_type, and even discusses when it's appropriate to leave it out.

Using void as the return type of a subprogram is explicitly represented as omitting DW_AT_type from the subprogram DIE, and that's probably where the nullptr idea came from.

A typedef of void * goes to a DW_TAG_pointer_type which is required to have at DW_AT_type, which I think we do emit as DW_TAG_unspecified_type. So, for consistency I'd think we might want typedef to do something similar. But, omitting is legal.