Page MenuHomePhabricator

Support DebugInfo generation for auto return type for C++ functions.
ClosedPublic

Authored by awpandey on Nov 20 2019, 9:31 PM.

Details

Summary

This patch will provide support for auto return type for the C++ member functions. Before this return type of the member function is deduced and store in the DIE.

Diff Detail

Event Timeline

awpandey created this revision.Nov 20 2019, 9:31 PM

Thanks! Couple of minor comments inline.

clang/lib/CodeGen/CGDebugInfo.cpp
822 ↗(On Diff #230371)

DBuilder.createUnspecifiedType("auto")

clang/test/CodeGenCXX/debug-info-auto-return.cpp
2 ↗(On Diff #230371)

What does this comment mean? This should work regardless of optimization level, right?

8 ↗(On Diff #230371)

Can you also CHECK that it is attached to the node we're expecting it to?

llvm/test/DebugInfo/X86/debug-info-auto-return.ll
2

I think you may need %llc_dwarf here to avoid problems on Windows bots?

111

please remove all unnecessary (quoted) attributes. This makes the test easier to maintain and read.

awpandey updated this revision to Diff 230601.Nov 22 2019, 12:52 AM
awpandey marked 3 inline comments as done.

This revision includes

  1. Added check for return type it is attached to the exact node we're expecting it to.
  2. Removed unnecessary strings and checks.
awpandey marked 2 inline comments as done.Nov 22 2019, 12:54 AM

Thanks @aprantl for your suggestions. I have revised by patch based on that.

aprantl added inline comments.Nov 22 2019, 8:34 AM
clang/lib/CodeGen/CGDebugInfo.cpp
2925 ↗(On Diff #230601)

You need to mark this LLVM_FALLTHROUGH now or you'll get a warning.

3104 ↗(On Diff #230601)

LLVM coding style doesn't put single statements into {}

3106 ↗(On Diff #230601)

same here

clang/test/CodeGenCXX/debug-info-auto-return.cpp
7 ↗(On Diff #230601)

Please don't hardcode the MDNode numbers, they will inevitably change over time. Instead use variables:

type: ![[SUBROUTINE_TYPE:[0-9]+]]`
// CHECK: ![[SUBROUTINE_TYPE]] =  !DISubroutineType(types: ![[ARGS:[0-9]+)

etc

probinson added inline comments.
llvm/test/DebugInfo/X86/debug-info-auto-return.ll
31

No need to over-specify this (form/index etc) as those details can change over time and aren't relevant to the test. Sufficient to check
DW_AT_linkage_name {{.*}} "_ZN7myClass7findMaxEv"

aprantl added inline comments.Nov 22 2019, 11:28 AM
llvm/test/DebugInfo/X86/debug-info-auto-return.ll
2

I don't think the FORMs are relevant for this test, so we can do without the -v

Generally if you're not touching LLVM code, I would suggest not adding an LLVM test - unless this is new or surprising usage of existing LLVM functionality (or the functionality was otherwise undertested in LLVM previously). But I'm guessing that's not the case here?

Generally if you're not touching LLVM code, I would suggest not adding an LLVM test - unless this is new or surprising usage of existing LLVM functionality (or the functionality was otherwise undertested in LLVM previously). But I'm guessing that's not the case here?

The rephrase the question David is asking: Do we already have an LLVM test for a named unspecified type?

awpandey updated this revision to Diff 231208.Nov 27 2019, 3:33 AM
awpandey marked 5 inline comments as done.

Hi @aprantl and @dblaikie. Currently, there is no test case for the unspecified type, so I have added that in the regression test suite. Also, I have incorporated all of your suggestions.

aprantl added inline comments.Dec 2 2019, 11:24 AM
clang/lib/CodeGen/CGDebugInfo.cpp
2925 ↗(On Diff #230601)

The FALLTHROUGH must come right before the next case

clang/test/CodeGenCXX/debug-info-auto-return.cpp
7 ↗(On Diff #230601)

you still need to make sure that the two nodes CHECKed are being connected. Please use variables like in the example I posted.

Hi @aprantl and @dblaikie. Currently, there is no test case for the unspecified type, so I have added that in the regression test suite.

It looks to me like there are a few tests for unspecified_type already:

$ grep -r unspecified_type llvm/test
llvm/test/Assembler/debug-info.ll:; CHECK-NEXT: !7 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
llvm/test/Assembler/debug-info.ll:!8 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
llvm/test/DebugInfo/COFF/types-std-nullptr-t.ll:!6 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
llvm/test/DebugInfo/X86/template.ll:; VERIFY-NOT: error: DIE has DW_AT_type with incompatible tag DW_TAG_unspecified_type
llvm/test/DebugInfo/X86/template.ll:; CHECK: [[NULLPTR]]:{{ *}}DW_TAG_unspecified_type
llvm/test/DebugInfo/X86/template.ll:!34 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")

@aprantl @probinson - I'm going to voice a little more objection to this feature as I think I did when it was being discussed for DWARF standardization - feel free to push back/veto me/etc: Describing this seems of limited value to consumers - the function cannot be called if the return type hasn't been resolved. I think modeling these sort of situations the same way member function templates are would be fine - omit the declaration entirely in translation units that have no function definition for an auto-returning function.

The only difference between this and templates is that we /can/ describe it & it can be useful for overload resolution - but the same is true of all functions (non-member, member, etc) & no DWARF producer I know of produces all function declarations into their output, so I'm not sure "make overload resolution work" is really a worthwhile goal. (without template descripttions (not the instantiations, but the templates themselves which we currently have no way of describing even if we wanted to) I don't think it's actually achievable)

clang/test/CodeGenCXX/debug-info-auto-return.cpp
12–15 ↗(On Diff #231208)

I'd probably make this a struct, rather than a class with a public member - access modifiers aren't relevant to this test, so far as I can tell, so might as well just have everything public by using a struct.

13 ↗(On Diff #231208)

No need for any member variables, I think?

20–23 ↗(On Diff #231208)

Could strip out the body here - leave a single "return 0;" in there if that's useful/needed.

llvm/test/DebugInfo/X86/debug-info-auto-return.ll
6–26

This code seems more complicated than it needs to be (it doesn't need any logic in the body of the functions, for instance, I think? Nor any member variables, nor 'main', etc) & the indentation should be fixed/made consistent.

@shafik Can you speak about whether this feature ("auto" return type in DWARF v5 section 5.2) would be useful for LLDB?

awpandey updated this revision to Diff 232281.Dec 5 2019, 2:06 AM
awpandey marked 5 inline comments as done.
This comment was removed by awpandey.
awpandey marked an inline comment as done.Dec 5 2019, 2:07 AM
awpandey marked an inline comment as done.Dec 5 2019, 2:56 AM

It looks to me like there are a few tests for unspecified_type already:

$ grep -r unspecified_type llvm/test
llvm/test/Assembler/debug-info.ll:; CHECK-NEXT: !7 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
llvm/test/Assembler/debug-info.ll:!8 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")

...
@dblaikie, are you suggesting me to modify some of these existing test cases to include auto return functionality as well.

clang/test/CodeGenCXX/debug-info-auto-return.cpp
9 ↗(On Diff #232281)

@aprantl I have tried to use variable as shown by you for showing links between the associated nodes.

It looks to me like there are a few tests for unspecified_type already:

$ grep -r unspecified_type llvm/test
llvm/test/Assembler/debug-info.ll:; CHECK-NEXT: !7 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
llvm/test/Assembler/debug-info.ll:!8 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")

...
@dblaikie, are you suggesting me to modify some of these existing test cases to include auto return functionality as well.

Nah - I'm suggesting that the existing coverage is sufficient and there's no need for an LLVM test for "auto return" - the specific string name used in a DW_TAG_unspecified_type is not parsed or processed in any way by LLVM - it's produced verbatim into the output (in the name of the unspecified_type) so there's no need to test with different strings (this would be akin to having a printf test case for "decltype(nullptr)" and then adding a different test case for printing "auto" - they're not meaningfully different test cases for printf (it's highly unlikely that a bug exists that can print one and not the other - and why those two strings in particular, among all the possible strings you could print that should be equally well handled?)).

@shafik Can you speak about whether this feature ("auto" return type in DWARF v5 section 5.2) would be useful for LLDB?

I agree with @dblaikie here, I can't come up with a scenario where this would be required. Unless like he mentioned we emit all the function declarations, which we don't.

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

The actual return type is known in a compile_unit where the method is defined, and not known in other compile_units. If the non-defining compile_units omit the return type, that means "void" not "auto". That is, one compile unit says it returns "void" and another compile unit says it returns something else. That is the inconsistency I meant.

If we use unspecified_type instead of omitting the return type, then a consumer knows that the method returns *something*, but it will have to look elsewhere to determine what that is.

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

The actual return type is known in a compile_unit where the method is defined, and not known in other compile_units. If the non-defining compile_units omit the return type, that means "void" not "auto". That is, one compile unit says it returns "void" and another compile unit says it returns something else. That is the inconsistency I meant.

If we use unspecified_type instead of omitting the return type, then a consumer knows that the method returns *something*, but it will have to look elsewhere to determine what that is.

Yeah, my argument was to omit the declarations of auto-returning functions entirely except in cases where their definition is available (either only when the definition is emitted, or whenever the definition's available so the deduced return type can be provided), same as a template instantiation.

So the class would be missing the declaration of the function in cases where the definition isn't available - same as templates and implicit special members.

(yeah, you could take this to its logical extreme and say we should treat all member functions this way - never produce a whole class definition enumerating all members - Eric and I have bandied that idea about as "classes as namespaces" more or less (the members in a class definition in one CU aren't necessarily the same as those in another, and a consumer would have to consider all of them together like it would have to for namespaces))

& honestly it's going to be pretty uncommon that any of this comes up - people aren't likely to separate their auto-returning function declaration from its definition. It'd be awkward to read code like that.

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

The actual return type is known in a compile_unit where the method is defined, and not known in other compile_units. If the non-defining compile_units omit the return type, that means "void" not "auto". That is, one compile unit says it returns "void" and another compile unit says it returns something else. That is the inconsistency I meant.

If we use unspecified_type instead of omitting the return type, then a consumer knows that the method returns *something*, but it will have to look elsewhere to determine what that is.

Yeah, my argument was to omit the declarations of auto-returning functions entirely except in cases where their definition is available (either only when the definition is emitted, or whenever the definition's available so the deduced return type can be provided), same as a template instantiation.

So the class would be missing the declaration of the function in cases where the definition isn't available - same as templates and implicit special members.

Understood. The class descriptions are inconsistent (in that sense) across CUs. This means deduplication in the type-unit sense would be less effective, but in the template-member and special-member cases there's really not much to be done about it. This argument says that auto-return methods aren't fundamentally different from those.

(yeah, you could take this to its logical extreme and say we should treat all member functions this way - never produce a whole class definition enumerating all members - Eric and I have bandied that idea about as "classes as namespaces" more or less (the members in a class definition in one CU aren't necessarily the same as those in another, and a consumer would have to consider all of them together like it would have to for namespaces))

Heh. Implemented that locally years ago as a space-saving measure; unreferenced methods are omitted. Our debugger internally merges the different descriptions that it encounters. In the compiler, the trick is that you don't actually know which methods to suppress until IRGen is complete, so I had to invent a Suppressed flag on the DISubprogram, that causes DwarfDebug to ignore it.

& honestly it's going to be pretty uncommon that any of this comes up - people aren't likely to separate their auto-returning function declaration from its definition. It'd be awkward to read code like that.

If it's a class method, which is the only case of interest (otherwise the unused declaration is suppressed anyway), the auto-return method is in the class declaration and all users of the header will have to see it. Most likely it would be a private method and used only in the implementation module, but still.

@probinson I was reading the C++ "auto" return type issue and I can't come up with a case where we don't have class descriptions across compilation units that are not consistent wrt to auto return type. Do you have a specific example?

The actual return type is known in a compile_unit where the method is defined, and not known in other compile_units. If the non-defining compile_units omit the return type, that means "void" not "auto". That is, one compile unit says it returns "void" and another compile unit says it returns something else. That is the inconsistency I meant.

If we use unspecified_type instead of omitting the return type, then a consumer knows that the method returns *something*, but it will have to look elsewhere to determine what that is.

Yeah, my argument was to omit the declarations of auto-returning functions entirely except in cases where their definition is available (either only when the definition is emitted, or whenever the definition's available so the deduced return type can be provided), same as a template instantiation.

So the class would be missing the declaration of the function in cases where the definition isn't available - same as templates and implicit special members.

Understood. The class descriptions are inconsistent (in that sense) across CUs. This means deduplication in the type-unit sense would be less effective, but in the template-member and special-member cases there's really not much to be done about it. This argument says that auto-return methods aren't fundamentally different from those.

I mean we could do something about them, to a degree - we could compute which special members are valid & emit them pre-emptively (though that'd be a bit tricky to do reliably, without unnecessarily instantiating any extra code, etc - but it's possible) & we could improve DWARF to have some description of templates. There still probably wouldn't be enough info to get overload resolution right, etc.

(yeah, you could take this to its logical extreme and say we should treat all member functions this way - never produce a whole class definition enumerating all members - Eric and I have bandied that idea about as "classes as namespaces" more or less (the members in a class definition in one CU aren't necessarily the same as those in another, and a consumer would have to consider all of them together like it would have to for namespaces))

Heh. Implemented that locally years ago as a space-saving measure; unreferenced methods are omitted. Our debugger internally merges the different descriptions that it encounters. In the compiler, the trick is that you don't actually know which methods to suppress until IRGen is complete, so I had to invent a Suppressed flag on the DISubprogram, that causes DwarfDebug to ignore it.

Oh, I'd just implement it like the template/implicit special member - the DISubprograms are built as-needed/only referenced from the llvm::Function, they don't appear in the "members" list of the DICompositeType at all, and then DwarfDebug attaches any that make it to LLVM's CodeGen/need to be emitted then.

If you comment out this call: https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CGDebugInfo.cpp#L2296 you can probably get the behavior you want without any other changes? (tested that, and it reduced the dwarfdump debug_info dumped output from 3.5k lines to 1k lines)

Perhaps we should implement this mode under -fno-standalone-debug (or something more aggressive) since "standalone" is one of the only places I can think of where having the full class definition would be handy - it'd make it clear that there is a specific function overload that can be called & the debugger could mangle the name and find the symbol in another TU that was built without debug info... not sure if any implement that, though. (that wouldn't be quite possible with an auto-returning function - you'd need to know the return type to figure out the calling ABI/handle the return value)

& honestly it's going to be pretty uncommon that any of this comes up - people aren't likely to separate their auto-returning function declaration from its definition. It'd be awkward to read code like that.

If it's a class method, which is the only case of interest (otherwise the unused declaration is suppressed anyway), the auto-return method is in the class declaration and all users of the header will have to see it. Most likely it would be a private method and used only in the implementation module, but still.

Perhaps we should implement this mode under -fno-standalone-debug (or something more aggressive) since "standalone" is one of the only places I can think of where having the full class definition would be handy

You'd also want it for type units, so they deduplicate more reliably? And probably should be DWARF only, not CodeView? But yeah, I'm down for it.

This tactic would depend on the debuggers knowing they are unlikely to get a full class description and shouldn't throw away any class they've "already seen." I remember that was an issue with, uh, one of those other debuggers that I never use. They might take the first one they see, and shrug off any others as not contributing anything really important. That would have to change.

Perhaps we should implement this mode under -fno-standalone-debug (or something more aggressive) since "standalone" is one of the only places I can think of where having the full class definition would be handy

You'd also want it for type units, so they deduplicate more reliably? And probably should be DWARF only, not CodeView? But yeah, I'm down for it.

It'd basically remove the need for type units entirely (& the DWARF would end up looking quite similar to DWARF using type units, just without the type units (well, you'd move the member variables from the type DIE In the type unit to the "skeleton" type DIE in the CU - but the member function handling looks very similar (because the skeleton type DIE ends up needing all the declarations of member functions in defined in the CU anyway - so the out of line definitions can refer to the member function declarations, their parameters, etc (since you can't refer to specific DIEs other than the total type DIE in a type unit)))).

Yeah, only for DWARF/I have no idea what CodeView can/would do.

This tactic would depend on the debuggers knowing they are unlikely to get a full class description and shouldn't throw away any class they've "already seen." I remember that was an issue with, uh, one of those other debuggers that I never use. They might take the first one they see, and shrug off any others as not contributing anything really important. That would have to change.

My understanding with GDB is that it tends to treat the types in independent CUs as being potentially independent - you can violate the ODR and see that reflected in the type descriptions depending on where you print the type from (eg: print the type while you're debugging one function in one TU and you get that TU's view of the type, then step into a function in another TU and print the type and you get that TU's view of the type).

Yeah, it's not high on my list & starting with the DWARF consumers would be the right place.

awpandey updated this revision to Diff 233289.Dec 11 2019, 2:39 AM

@dblaikie . I have removed the redundant test case. What else should I do in this patch?

@dblaikie . I have removed the redundant test case. What else should I do in this patch?

Please address this warning before committing:

/usr/local/google/home/blaikie/dev/llvm/src/clang/lib/CodeGen/CGDebugInfo.cpp:3100:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case Type::Attributed:
  ^
/usr/local/google/home/blaikie/dev/llvm/src/clang/lib/CodeGen/CGDebugInfo.cpp:3100:3: note: insert 'break;' to avoid fall-through
  case Type::Attributed:
  ^
  break; 
1 warning generated.

It looks like this implementation is a bit buggy in one way and incomplete in another:

  1. even if the auto-returning function is defined, that function definition doesn't describe the concrete return type. Compare GCC and Clang's output for your example and note that... oh.

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?) GCC does use the unspecified type "auto" even back in DWARFv4 and it leaves the subprogram definition DIE without a DW_AT_type if the auto type ends up as void (what else could it do?) so I guess we can do this for consistency & consumers have to know that a definition can't really have an auto return type and that it must be really void.

In any case - change the test case to use a non-void return type in the definition ("return 3;" for instance, to get an int return type instead) and check that the DISubprogram for the definition has a concrete return type of "int" while the DISubprogram for the declaration has the "auto" unspecified_type return type. (contrast/test against GCC's behavior)

  1. Presumably in a follow-up patch, make sure that the declaration for the DISubprogram declaration for an "auto" return type function appears in the member list of the DICompositeType even if the function is not called (same as other normal (non-implicit/non-template) functions) since that's the value of being able to describe the return type as "auto" (the function can be described even when the definition isn't available/emitted) - it doesn't currently. (contrast/test against with GCC's behavior)

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?)

Normally, the DW_AT_specification on the definition would mean, look at the declaration for additional attributes, such as DW_AT_type. However, the declaration's unspecified_type means, look at the definition. The definition omits DW_AT_type, therefore the return type is "void".
It's a wee bit circular, but I think it's not unreasonable to expect the consumer to figure this out.

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?)

Normally, the DW_AT_specification on the definition would mean, look at the declaration for additional attributes, such as DW_AT_type. However, the declaration's unspecified_type means, look at the definition. The definition omits DW_AT_type, therefore the return type is "void".
It's a wee bit circular, but I think it's not unreasonable to expect the consumer to figure this out.

Fair enough. I think that's pretty awkward, but don't feel strongly enough to say that's bad/wrong - it's what GCC does, it's probably what GDB understands, etc.

Test cases/functionality need to be fixed here, because currently the deduced type isn't being carried on the function definition (it just "works out" for the limited testing provided - because the test is testing void return, but adding/changing the test to test non-void return should expose the bug)

Also - I think this probably should be enabled even pre-DWARFv5. GDB does that and unspecified_type is available pre-v5 & it saves having a divergence in the frontend for v5 generation which I think is probably good (I don't think we have that as a hard line, but I'd tend to think it's a direction we should generally lean in - where the DWARF version is handled by the metadata attribute and the backend for the most part/where possible).

It looks like this implementation is a bit buggy in one way and incomplete in another:

  1. even if the auto-returning function is defined, that function definition doesn't describe the concrete return type. Compare GCC and Clang's output for your example and note that... oh.

I think that's correct behavior, consider this for a moment --

struct foo {
auto foo_func();
};
int foo::foo_func(){return 0;}
clang error: ->
error: return type of out-of-line definition of 'foo::foo_func' differs from that in the declaration

So this seems fair to me, regardless of the concrete return type{assuming this is what this patch is doing}. We should be emitting auto in declaration. AKA in unspecified_type. GCC(trunk) also seems fair here.

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?)

I'm a bit confused here, regardless of the concrete return type{void/int/float} GCC(trunk) is emitting
DW_TAG_unspecified_type
DW_AT_name "auto"
Are you trying to say we should be emitting DW_AT_type void/int/float ?? That's what functionality of clang is right now, and if we entertain that, then their is no point of emitting DW_TAG_unspecifed_type auto at first place.

GCC does use the unspecified type "auto" even back in DWARFv4 and it leaves the subprogram definition DIE without a DW_AT_type if the auto type ends up as void (what else could it do?) so I guess we can do this for consistency & consumers have to know that a definition can't really have an auto return type and that it must be really void.

In any case - change the test case to use a non-void return type in the definition ("return 3;" for instance, to get an int return type instead) and check that the DISubprogram for the definition has a concrete return type of "int" while the DISubprogram for the declaration has the "auto" unspecified_type return type. (contrast/test against GCC's behavior)

  1. Presumably in a follow-up patch, make sure that the declaration for the DISubprogram declaration for an "auto" return type function appears in the member list of the DICompositeType even if the function is not called (same as other normal (non-implicit/non-template) functions) since that's the value of being able to describe the return type as "auto" (the function can be described even when the definition isn't available/emitted) - it doesn't currently. (contrast/test against with GCC's behavior)

Agreed testing for this must be exhaustive, but I think for all test cases behavior should be same -- "DW_TAG_unspecified_type auto" should be emitted for the function declared/defined as auto returnning. Do you have other test cases in mind, where above points diverges ??

"DW_TAG_unspecified_type auto" should be emitted for the function declared/defined as auto returnning. Do you have other test cases in mind, where above points diverges ??

The declaration would have DW_AT_type point to DW_TAG_unspecified_type, but the definition should have DW_AT_type use the actual type (which is now known, because you have the definition). The "actual type" might be "void", in which case the definition would omit DW_AT_type, just like a normal (non-auto) function that returns void.

It looks like this implementation is a bit buggy in one way and incomplete in another:

  1. even if the auto-returning function is defined, that function definition doesn't describe the concrete return type. Compare GCC and Clang's output for your example and note that... oh.

I think that's correct behavior, consider this for a moment --

struct foo {
auto foo_func();
};
int foo::foo_func(){return 0;}
clang error: ->
error: return type of out-of-line definition of 'foo::foo_func' differs from that in the declaration

So this seems fair to me, regardless of the concrete return type{assuming this is what this patch is doing}. We should be emitting auto in declaration. AKA in unspecified_type. GCC(trunk) also seems fair here.

Sorry, the example I had in mind was this:

struct type {
  auto func();
};
auto type::func() {
  return 3;
}

Which GCC produces:

DW_TAG_structure_type
  DW_AT_name ("type")
  DW_TAG_subprogram
    DW_AT_name ("func")
    DW_AT_type (DW_TAG_unspecified_type "auto")
    ...
DW_TAG_subprogram
  DW_AT_specification (... ^)
  DW_AT_type (DW_TAG_base_type "int")
  ...

(this should be the same debug info even if the function is defined inline in the class, rather than defined out of line (assuming the function's called - which produces a definition))

Hmm, maybe this feature/suggestion is broken or at least not exactly awesome when it comes to auto-returning functions that are eventually void-returning functions? Now the function definition has no DW_AT_type to override the unspecified_type in the declaration... :/ that's unfortunate (@probinson - thoughts?)

I'm a bit confused here, regardless of the concrete return type{void/int/float} GCC(trunk) is emitting
DW_TAG_unspecified_type
DW_AT_name "auto"
Are you trying to say we should be emitting DW_AT_type void/int/float ?? That's what functionality of clang is right now, and if we entertain that, then their is no point of emitting DW_TAG_unspecifed_type auto at first place.

As @probinson said - for the definition DIE/DISubprogram the type should be the concrete, deduced type. For the declaration DIE/DISubprogram, the return type should be "auto".

awpandey updated this revision to Diff 235998.Jan 3 2020, 12:40 AM

@dblaikie Thanks a lot for the suggestions. I have made the changes such that this will become consistent with your suggestions.

dblaikie added inline comments.Jan 8 2020, 2:33 PM
clang/lib/CodeGen/CGDebugInfo.cpp
1467–1468 ↗(On Diff #235998)

Looks like this patch probably doesn't address the case where the function is static. Though adding support for static should be done in a separate/next patch. It might turn out that handling that case motivates sinking this functionality further down into more shared functionality, but we can deal with that in the review for that separate patch.

(also, there's work being done to add various non-member function declarations to LLVM debug info emission (for call site debug info) - at some point (not in this patch) it's probably worth checking whether those non-member function declarations might need this sort of handling as well)

clang/test/CodeGenCXX/debug-info-auto-return.cpp
9 ↗(On Diff #235998)

This looks incorrect - I expect what you want here is [[t]] to match the previously defined t on the prior CHECK. (similarly with the other matches - use [[x:pattern]] to define the pattern, then [[x]] to reference the previously defined pattern - otherwise the two won't be associated (if you don't need any association between two pattern matches, you can use {{pattern}} for unnamed pattern matching))

Also, please use descriptive names for these matches & I /think/ the convention is usually upper case? I'd think "DECL_TYPE", "DEF_TYPE", "AUTO_TYPE", and "DOUBLE_TYPE" might be good names.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1168–1175

Please split this out into a separate patch/review with its own LLVM-level test (& this would be committed before the clang-side)

awpandey updated this revision to Diff 237264.Jan 10 2020, 1:16 AM

@dblaikie I have added an LLVM test case and I will commit it like a different patch.
I have also updated the clang test case.

dblaikie accepted this revision.Jan 10 2020, 9:27 AM

Looks good - thanks!

This revision is now accepted and ready to land.Jan 10 2020, 9:27 AM
This revision was automatically updated to reflect the committed changes.

This seems to have introduced a crash compiling libcxx. I'm currently reducing the crashing code.

Following crashes for me with clang++ -g2:

struct Thing {
  void crash(Thing &Other) {
    auto Lambda = [&] {
      Other.external();
    };
    Lambda();
  }
  void external();
};

void test(Thing &A, Thing &B) {
  A.crash(B);
}

Haven't yet verified that it's this commit, but certainly looks related. Will revert once this is confirmed.

Following crashes for me with clang++ -g2:

This crashes at 6d6a4590c5d4 but not at c958639098a8. Reverted in e45fcfc3aa57bb237fd4fd694d0c257be66d5482, up to you whether the prev patch should be reverted too.

Stack trace:

1.	<eof> parser at end of file
2.	Code generation
 #0 0x000000000263ea84 PrintStackTrace /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/Support/Unix/Signals.inc:564:13
 #1 0x000000000263ea84 PrintStackTraceSignalHandler(void*) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/Support/Unix/Signals.inc:624:0
 #2 0x000000000263c61e llvm::sys::RunSignalHandlers() /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/Support/Signals.cpp:69:18
 #3 0x000000000263ee9c SignalHandler(int) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/Support/Unix/Signals.inc:396:3
 #4 0x00007fc9c4fad3a0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0)
 #5 0x00007fc9c424ecfb raise (/lib/x86_64-linux-gnu/libc.so.6+0x36cfb)
 #6 0x00007fc9c42398ad abort (/lib/x86_64-linux-gnu/libc.so.6+0x218ad)
 #7 0x00007fc9c423977f (/lib/x86_64-linux-gnu/libc.so.6+0x2177f)
 #8 0x00007fc9c4247542 (/lib/x86_64-linux-gnu/libc.so.6+0x2f542)
 #9 0x00000000030c08a6 (/usr/local/google/home/sammccall/llvmbuild-mono/bin/clang-10+0x30c08a6)
#10 0x00000000030c5b7e llvm::DwarfUnit::applySubprogramDefinitionAttributes(llvm::DISubprogram const*, llvm::DIE&) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1176:15
#11 0x00000000030c4e46 llvm::DwarfUnit::applySubprogramAttributes(llvm::DISubprogram const*, llvm::DIE&, bool) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1221:9
#12 0x0000000003106a0f getOperand /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/IR/Metadata.h:1078:5
#13 0x0000000003106a0f getOperandAs<llvm::MDString> /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/IR/DebugInfoMetadata.h:132:0
#14 0x0000000003106a0f getStringOperand /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/IR/DebugInfoMetadata.h:136:0
#15 0x0000000003106a0f getName /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/IR/DebugInfoMetadata.h:1793:0
#16 0x0000000003106a0f llvm::DwarfCompileUnit::applySubprogramAttributesToDefinition(llvm::DISubprogram const*, llvm::DIE&) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1299:0
#17 0x00000000030a01d2 getSkeleton /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:116:12
#18 0x00000000030a01d2 forBothCUs<(lambda at /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1090:9)> /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:509:0
#19 0x00000000030a01d2 llvm::DwarfDebug::finishSubprogramDefinitions() /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1088:0
#20 0x00000000030a029a llvm::DwarfDebug::finalizeModuleInfo() /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1099:3
#21 0x00000000030a0978 llvm::DwarfDebug::endModule() /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1238:3
#22 0x0000000003084da3 ~TimeRegion /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/Support/Timer.h:152:9
#23 0x0000000003084da3 llvm::AsmPrinter::doFinalization(llvm::Module&) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1485:0
#24 0x0000000001fc9fa5 llvm::FPPassManager::doFinalization(llvm::Module&) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/IR/LegacyPassManager.cpp:1535:13
#25 0x0000000001fca5f6 runOnModule /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/IR/LegacyPassManager.cpp:1611:41
#26 0x0000000001fca5f6 llvm::legacy::PassManagerImpl::run(llvm::Module&) /usr/local/google/home/sammccall/src/llvm-mono/llvm/lib/IR/LegacyPassManager.cpp:1694:0
#27 0x0000000002824eb9 ~TimeTraceScope /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/Support/TimeProfiler.h:74:35
#28 0x0000000002824eb9 EmitAssembly /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/CodeGen/BackendUtil.cpp:912:0
#29 0x0000000002824eb9 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/CodeGen/BackendUtil.cpp:1549:0
#30 0x00000000033d5b45 ~unique_ptr /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/unique_ptr.h:273:6
#31 0x00000000033d5b45 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/CodeGen/CodeGenAction.cpp:312:0
#32 0x0000000003cc13f3 __normal_iterator /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/stl_iterator.h:784:20
#33 0x0000000003cc13f3 begin /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/stl_vector.h:699:0
#34 0x0000000003cc13f3 finalize<std::vector<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> >, std::allocator<std::unique_ptr<clang::TemplateInstantiationCallback, std::default_delete<clang::TemplateInstantiationCallback> > > > > /usr/local/google/home/sammccall/src/llvm-mono/clang/include/clang/Sema/TemplateInstCallback.h:54:0
#35 0x0000000003cc13f3 clang::ParseAST(clang::Sema&, bool, bool) /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/Parse/ParseAST.cpp:178:0
#36 0x0000000002dcdb50 clang::FrontendAction::Execute() /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/Frontend/FrontendAction.cpp:940:10
#37 0x0000000002d66974 getPtr /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/Support/Error.h:273:42
#38 0x0000000002d66974 operator bool /usr/local/google/home/sammccall/src/llvm-mono/llvm/include/llvm/Support/Error.h:236:0
#39 0x0000000002d66974 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/Frontend/CompilerInstance.cpp:965:0
#40 0x0000000002e7d21f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /usr/local/google/home/sammccall/src/llvm-mono/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:290:25
#41 0x0000000000a353fb cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /usr/local/google/home/sammccall/src/llvm-mono/clang/tools/driver/cc1_main.cpp:239:15
#42 0x0000000000a33007 ExecuteCC1Tool /usr/local/google/home/sammccall/src/llvm-mono/clang/tools/driver/driver.cpp:309:12
#43 0x0000000000a33007 main /usr/local/google/home/sammccall/src/llvm-mono/clang/tools/driver/driver.cpp:382:0
#44 0x00007fc9c423b52b __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2352b)
#45 0x0000000000a3025a _start (/usr/local/google/home/sammccall/llvmbuild-mono/bin/clang-10+0xa3025a)
clang-10: error: unable to execute command: Aborted
clang-10: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 10.0.0 (git@github.com:llvm/llvm-project.git 6d6a4590c5d4c7fc7445d72fe685f966b0a8cafb)
Target: x86_64-unknown-linux-gnu
Thread model: posix

Thanks @sammccall , for the test case. Seems like, this implementation doesn't address the lambda functions returning "auto". We're looking into this.

This also broke a number of the tests on the Windows LLDB bot, so when you get around to resubmitting the change with a fix, please make sure the bot doesn't get broken again: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/12551

Looks like the first commit c958639098a8 is also causing a crash in chromium code, so I will revert that as well.
I got a reduced test case:

typedef void voidtype;
struct S {
  voidtype a();
};
void S::a() {}