[Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

[shchenz]

Summary

As discussed in D99250 , we added a new tuning debugger DBX(D99400) to add some debug info customizations for DBX on AIX.

This is part of the customizations, not generating DWARF infos not matching the DWARF version.

This is for DW_TAG_rvalue_reference_type tag.

Now for DBX, we only generate this tag when DWARF version is 4 or higher.

[probinson]

If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting -gstrict-dwarf and just have DBX tuning imply that.

[jsji]

In D100630#2694681, @probinson wrote:

If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting -gstrict-dwarf and just have DBX tuning imply that.

Thanks! Yes, that is a good idea. By adding -gstrict-dwarf, all these dwarf version check work might be beneficial to others, not just for *dbx* .

[shchenz]

In D100630#2694681, @probinson wrote:

If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting -gstrict-dwarf and just have DBX tuning imply that.

Hi @probinson thanks for pointing out the way, agree with this new solution. After checking the codes, I found there are already -gstrict-dwarf, -gno-strict-dwarf options in clang code base, but there seem no users of them. Do you happen to know the story of these two options? I get a quick search, no clue yet. Can we reuse these options for the intention here. Thanks?

[dblaikie]

In D100630#2697728, @shchenz wrote:

In D100630#2694681, @probinson wrote:

If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting -gstrict-dwarf and just have DBX tuning imply that.

Hi @probinson thanks for pointing out the way, agree with this new solution. After checking the codes, I found there are already -gstrict-dwarf, -gno-strict-dwarf options in clang code base, but there seem no users of them. Do you happen to know the story of these two options? I get a quick search, no clue yet. Can we reuse these options for the intention here. Thanks?

I'd suggest checking the history of the commits that added them - but likely they were added for command line compatibility with gcc but I expect they're currently no-ops in Clang. Adding the expected functionality to them seems fine to me.

[shchenz]

In D100630#2697732, @dblaikie wrote:

In D100630#2697728, @shchenz wrote:

In D100630#2694681, @probinson wrote:

If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting -gstrict-dwarf and just have DBX tuning imply that.

Hi @probinson thanks for pointing out the way, agree with this new solution. After checking the codes, I found there are already -gstrict-dwarf, -gno-strict-dwarf options in clang code base, but there seem no users of them. Do you happen to know the story of these two options? I get a quick search, no clue yet. Can we reuse these options for the intention here. Thanks?

I'd suggest checking the history of the commits that added them - but likely they were added for command line compatibility with gcc but I expect they're currently no-ops in Clang. Adding the expected functionality to them seems fine to me.

Yes, it should be for compatibility with gcc. I found same options in gcc. I create patch D100809 for this support in clang. Could you please help to review? Thanks

[shchenz]

gentle ping.

[dblaikie]

Mixed feelings - is this the only place where we're changing behavior for strict-DWARF in the frontend/clang? (maybe it'd be better to do it all in the same place, down in the backend when the IR is mapped to actual DWARF)

@aprantl @probinson - you two have any thoughts on this layering aspect?

I think for CodeView we make some different choices up in the frontend (& completely different implementations in the backend), so it's not totally unreasonable, but curious how folks feel.

[shchenz]

update according to @dblaikie comments

[shchenz]

In D100630#2745272, @dblaikie wrote:

Mixed feelings - is this the only place where we're changing behavior for strict-DWARF in the frontend/clang? (maybe it'd be better to do it all in the same place, down in the backend when the IR is mapped to actual DWARF)

Update the patch to do the check down in the backend. Seems we already have one centralized interface for TAGs generation.

@aprantl @probinson - you two have any thoughts on this layering aspect?

I think for CodeView we make some different choices up in the frontend (& completely different implementations in the backend), so it's not totally unreasonable, but curious how folks feel.

Welcome your comments! @aprantl @probinson

[dblaikie]

Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

391 ↗

(On Diff #343983)

Rather than adding a cast here, perhaps the parameter type could be updated? (maybe in a separate preliminary commit, though, so as not to muddy this one)

[shchenz]

1: address @dblaikie comments

[shchenz]

In D100630#2748899, @dblaikie wrote:

Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types?

Yes, it will cause such issue in theory. I can not cook a case that has a DW_TAG_reference_type to a DW_TAG_rvalue_reference_types or has a DW_TAG_rvalue_reference_types to a DW_TAG_reference_type for now. Is there such usage that we both be rvalue-reference and lvalue-reference to a basic type?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

391 ↗

(On Diff #343983)

Add a new NFC patch D102207

[dblaikie]

In D100630#2749550, @shchenz wrote:

In D100630#2748899, @dblaikie wrote:

Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types?

Yes, it will cause such issue in theory. I can not cook a case that has a DW_TAG_reference_type to a DW_TAG_rvalue_reference_types or has a DW_TAG_rvalue_reference_types to a DW_TAG_reference_type for now. Is there such usage that we both be rvalue-reference and lvalue-reference to a basic type?

Ah, I was less thinking of a case of an rvalue reference and a normal reference in the same chain - I was thinking more about two unrelated reference types.

Try some code like:

void f1(int &&x, int &y) {
}

I'm guessing without this change - you get 'x's type pointing to the rvalue_reference_type, 'y's type pointing to the reference_type, and both of those pointing to the 'int' DW_TAG_basic_type.

But with this change I'm worried you'll get two copies of the DW_TAG_reference_type - one that replaces the rvalue reference, and then the real one. Instead you should get one copy that both 'x' and 'y' refer to, I think.

[shchenz]

In D100630#2749594, @dblaikie wrote:

In D100630#2749550, @shchenz wrote:

In D100630#2748899, @dblaikie wrote:

Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types?

Yes, it will cause such issue in theory. I can not cook a case that has a DW_TAG_reference_type to a DW_TAG_rvalue_reference_types or has a DW_TAG_rvalue_reference_types to a DW_TAG_reference_type for now. Is there such usage that we both be rvalue-reference and lvalue-reference to a basic type?

Ah, I was less thinking of a case of an rvalue reference and a normal reference in the same chain - I was thinking more about two unrelated reference types.

Try some code like:

void f1(int &&x, int &y) {
}

I'm guessing without this change - you get 'x's type pointing to the rvalue_reference_type, 'y's type pointing to the reference_type, and both of those pointing to the 'int' DW_TAG_basic_type.

But with this change I'm worried you'll get two copies of the DW_TAG_reference_type - one that replaces the rvalue reference, and then the real one. Instead you should get one copy that both 'x' and 'y' refer to, I think.

Hmm, yeah, indeed, a duplicated DW_TAG_reference_type is generated.

0x00000069:   DW_TAG_reference_type
                DW_AT_type      (0x0000006e "int")

0x0000006e:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000075:   DW_TAG_reference_type
                DW_AT_type      (0x0000006e "int")

I think rather than fixing the duplicated DW_TAG_reference_type in the backend, maybe it is better to fix this strict DWARF issue in the FE like the fix in the previous patch https://reviews.llvm.org/D100630?id=338755

What do you think? @dblaikie

[dblaikie]

In D100630#2749625, @shchenz wrote:

In D100630#2749594, @dblaikie wrote:

In D100630#2749550, @shchenz wrote:

In D100630#2748899, @dblaikie wrote:

Does this cause duplicate DW_TAG_reference_types in the output, if the input IR Has both DW_TAG_reference_type and DW_TAG_rvalue_reference_types?

Yes, it will cause such issue in theory. I can not cook a case that has a DW_TAG_reference_type to a DW_TAG_rvalue_reference_types or has a DW_TAG_rvalue_reference_types to a DW_TAG_reference_type for now. Is there such usage that we both be rvalue-reference and lvalue-reference to a basic type?

Ah, I was less thinking of a case of an rvalue reference and a normal reference in the same chain - I was thinking more about two unrelated reference types.

Try some code like:

void f1(int &&x, int &y) {
}

I'm guessing without this change - you get 'x's type pointing to the rvalue_reference_type, 'y's type pointing to the reference_type, and both of those pointing to the 'int' DW_TAG_basic_type.

But with this change I'm worried you'll get two copies of the DW_TAG_reference_type - one that replaces the rvalue reference, and then the real one. Instead you should get one copy that both 'x' and 'y' refer to, I think.

Hmm, yeah, indeed, a duplicated DW_TAG_reference_type is generated.

0x00000069:   DW_TAG_reference_type
                DW_AT_type      (0x0000006e "int")

0x0000006e:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x00000075:   DW_TAG_reference_type
                DW_AT_type      (0x0000006e "int")

I think rather than fixing the duplicated DW_TAG_reference_type in the backend, maybe it is better to fix this strict DWARF issue in the FE like the fix in the previous patch https://reviews.llvm.org/D100630?id=338755

What do you think? @dblaikie

I'd still hope this issue could be addressed in the backend, but yeah - it might be non-trivial.

@aprantl @probinson etc - any thoughts on this?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

398–399 ↗

(On Diff #344269)

side note: Not sure what this assertion is for/does?

[shchenz]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

398–399 ↗

(On Diff #344269)

typo :)

[aprantl]

Doing this in the backend SGTM, assuming all of @dblaikie's comments are addressed.

[dblaikie]

In D100630#2755936, @aprantl wrote:

Doing this in the backend SGTM, assuming all of @dblaikie's comments are addressed.

The complication is that if we naively lower DW_TAG_rvalue_reference to DW_TAG_reference, then we'll end up with two DW_TAG_reference DIEs in the output (one from the rvalue_reference, one from the reference) - which seems not ideal. I don't have /great/ ideas about how to address that, but think it might be worth figuring out - but doing it in the frontend is pretty simple (& CodeView's certainly got precedent for changing the DI metadata in the frontend - not sure if we have cases where we change the DI metadata based on DWARF version or anything like that - do you know of any?).

[shchenz]

1: add the FIXME for the duplicated tags issue

[shchenz]

Thanks for your comments! @dblaikie @aprantl . I think we have an agreement that we should handle the strict dwarf tag in the backend.

And for duplicated tags issue, since it may need a while to fix, I have added a TODO in the code change and also update to test case to show the duplicated tags issue and also a FIXME in the case.

Do you think it is good to treat it as a FIXME for now? I currently have no time to fix this, but I will keep this in mind and will fix this later when I get a better understanding of the DWARF codes. Thanks.

[dblaikie]

This isn't the sort of thing I'd like to leave to a FIXME later - seems like the sort of thing we shouldn't create now to fix later.

@aprantl mind having a second look at this to consider the situation further?

[shchenz]

In D100630#2760972, @dblaikie wrote:

This isn't the sort of thing I'd like to leave to a FIXME later - seems like the sort of thing we shouldn't create now to fix later.

@aprantl mind having a second look at this to consider the situation further?

OK. Let me summary the two solutions here:
1: fixed in the FE. See current patch.
Advantage:

• the DWARF info for DW_TAG_rvalue_reference_type and DW_TAG_reference_type is cached, so we will not have redundant info;
• FE also respects strict dwarf option;

Disadvantage:

• No common interface for strict dwarf option.

2: fixed in the BE. See https://reviews.llvm.org/D100630?vs=on&id=345333#toc.
Advantage:

• there is a common interface for tags adding, so we can add strict dwarf support in one place

Disadvantage:

• Redundant DWARF info since the final DWARF info adding in BE has no cache mechanism. And maybe there should be no such mechanism because after we add a DIE tag, we may add an attribute anywhere later, so it is hard to tell the DIE tag is identical when we create the DIE tag. If we optimize the redundant DWARF tag in the place where we add the fix, what about when we need to add another attribute to one of the identical DIE tags? Splitting them again into two different DIE tags?
• IR Metadata does not respect the strict DWARF option.

Welcome your comments? @dblaikie @aprantl @probinson and other reviewers.

[shchenz]

1: use the solution in the FE

[shchenz]

@dblaikie Hi David, should we change to use the FE solution? Maybe we should fix the issue where it happens? We should not let the FE generate the un-strict dwarf tag in FE and then fix it in the BE. I have changed this patch to fix this strict DWARF issue in FE. What do you think? Thanks

[dblaikie]

I'd still like to hear from some of the other folks mentioned previously.

(though there is precedent for debug info modes being frontend, like -gmlt, etc - so it's not totally novel/to be avoided, but I'd like the discussion first)

[shchenz]

OK, thanks David @dblaikie .

Ping for your comments, we need some discussion here to decide we should fix this in FE or BE. See more in https://reviews.llvm.org/D100630#2762354

@aprantl @probinson and other reviewers ^- ^

[aprantl]

If gracefully lowering rvalue references to references needs knowledge about the Clang typesystem (or depends on LLVM IR type uniquing to be efficient, as @dblaikie points out) then we should do it in the Clang frontend. As much as we can we should avoid encoding debug info format specific knowledge in the Clang frontend, but that's not always possible. I'm fine with taking this patch.

[dblaikie]

In D100630#2775207, @aprantl wrote:

If gracefully lowering rvalue references to references needs knowledge about the Clang typesystem (or depends on LLVM IR type uniquing to be efficient, as @dblaikie points out) then we should do it in the Clang frontend. As much as we can we should avoid encoding debug info format specific knowledge in the Clang frontend, but that's not always possible. I'm fine with taking this patch.

Fair enough then - thanks for chiming in!

[shchenz]

Thanks for your review! @dblaikie @aprantl

clang/lib/CodeGen/CGDebugInfo.cpp

 llvm::DIType *CGDebugInfo::CreateType(const LValueReferenceType *Ty,
                                       llvm::DIFile *Unit) {
   return CreatePointerLikeType(llvm::dwarf::DW_TAG_reference_type, Ty,
                                Ty->getPointeeType(), Unit);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const RValueReferenceType *Ty,
                                       llvm::DIFile *Unit) {
-  return CreatePointerLikeType(llvm::dwarf::DW_TAG_rvalue_reference_type, Ty,
-                               Ty->getPointeeType(), Unit);
+  llvm::dwarf::Tag Tag = llvm::dwarf::DW_TAG_rvalue_reference_type;
+  // DW_TAG_rvalue_reference_type was introduced in DWARF 4.
+  if (CGM.getCodeGenOpts().DebugStrictDwarf &&
+      CGM.getCodeGenOpts().DwarfVersion < 4)
+    Tag = llvm::dwarf::DW_TAG_reference_type;
+
+  return CreatePointerLikeType(Tag, Ty, Ty->getPointeeType(), Unit);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const MemberPointerType *Ty,
                                       llvm::DIFile *U) {
   llvm::DINode::DIFlags Flags = llvm::DINode::FlagZero;
   uint64_t Size = 0;
 
   if (!Ty->isIncompleteType()) {

clang/test/CodeGenCXX/debug-info-rvalue-ref.cpp

 // RUN: %clang_cc1 -std=c++11 -emit-llvm -debug-info-kind=limited -triple x86_64-apple-darwin %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -dwarf-version=4 -gstrict-dwarf -emit-llvm -debug-info-kind=limited \
+// RUN:   -triple x86_64-apple-darwin %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -dwarf-version=3 -gstrict-dwarf -emit-llvm -debug-info-kind=limited \
+// RUN:  -triple x86_64-apple-darwin %s -o - | FileCheck %s --check-prefix=NORVALUE
 
 extern "C" {
 extern int printf(const char * format, ...);
 }
 void foo (int &&i)
 {
   printf("%d\n", i);
 }
 
 // CHECK: !DIDerivedType(tag: DW_TAG_rvalue_reference_type, baseType: ![[INT:[0-9]+]], size: 64)
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
+// NORVALUE: !DIDerivedType(tag: DW_TAG_reference_type, baseType: ![[INT:[0-9]+]], size: 64)