CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location

[MaskRay]

Summary

For Clang synthesized __va_list_tag (CreateX86_64ABIBuiltinVaListDecl),
its DW_AT_decl_file/DW_AT_decl_line are arbitrarily set from CurLoc.

In a stage 2 -DCMAKE_BUILD_TYPE=Debug clang build, I observe that
in driver.cpp, DW_AT_decl_file/DW_AT_decl_line may be set to an #include line
(the transitively included file uses va_arg (__builtin_va_arg)).
This seems arbitrary. Drop that.

Depends on D94391

[MaskRay]

diff

-  [30] .debug_abbrev     PROGBITS        0000000000000000 ec7533c 616a22 00      0   0  1
-  [31] .debug_info       PROGBITS        0000000000000000 f28bd5e 50f9770b 00      0   0  1
-  [32] .debug_ranges     PROGBITS        0000000000000000 60223469 285e9b0 00      0   0  1
-  [33] .debug_str        PROGBITS        0000000000000000 62a81e19 ce211e0 01  MS  0   0  1
-  [34] .debug_line       PROGBITS        0000000000000000 6f8a2ff9 8a5ea2d 00      0   0  1
-  [35] .symtab           SYMTAB          0000000000000000 78301a28 156db60 18     37 821346  8
-  [36] .shstrtab         STRTAB          0000000000000000 7986f588 000168 00      0   0  1
-  [37] .strtab           STRTAB          0000000000000000 7986f6f0 69ab553 00      0   0  1
+  [30] .debug_abbrev     PROGBITS        0000000000000000 ec7533c 618ce4 00      0   0  1
+  [31] .debug_info       PROGBITS        0000000000000000 f28e020 50f97700 00      0   0  1
+  [32] .debug_ranges     PROGBITS        0000000000000000 60225720 285e9b0 00      0   0  1
+  [33] .debug_str        PROGBITS        0000000000000000 62a840d0 ce211e0 01  MS  0   0  1
+  [34] .debug_line       PROGBITS        0000000000000000 6f8a52b0 8a5ea2d 00      0   0  1
+  [35] .symtab           SYMTAB          0000000000000000 78303ce0 156db60 18     37 821346  8
+  [36] .shstrtab         STRTAB          0000000000000000 79871840 000168 00      0   0  1
+  [37] .strtab           STRTAB          0000000000000000 798719a8 69ab553 00      0   0  1

.debug_info is too large... I don't know how to compare it more effectively.

[dblaikie]

clang/lib/CodeGen/CGDebugInfo.cpp

3331–3332

Perhaps we can delete these two lines (using null/0 later on/where these values are referenced)? Since they aren't used for forward declarations anyway: https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfUnit.cpp#L814

[MaskRay]

clang/lib/CodeGen/CGDebugInfo.cpp

3331–3332

Making Line always-0 will change .debug_info size quite a bit. It seems that there may be degraded debug info.

 0x00000048:     DW_TAG_class_type
                   DW_AT_calling_convention     (DW_CC_pass_by_value)
                   DW_AT_name   ("StringRef")
                   DW_AT_byte_size      (0x10)
-                  DW_AT_decl_file      ("/home/maskray/llvm/llvm/include/llvm/ADT/StringRef.h")
-                  DW_AT_decl_line      (57)
 
-0x00000051:       DW_TAG_member
+0x0000004f:       DW_TAG_member
                     DW_AT_name ("npos")
-                    DW_AT_type (0x0000eceb "const size_t")
+                    DW_AT_type (0x0000eac6 "const size_t")
                     DW_AT_decl_file    ("/home/maskray/llvm/llvm/include/llvm/ADT/StringRef.h")
                     DW_AT_decl_line    (59)
                     DW_AT_external     (true)
                     DW_AT_declaration  (true)
                     DW_AT_accessibility        (DW_ACCESS_public)
                     DW_AT_const_value  (18446744073709551615)

[dblaikie]

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Seems like this change might lead to a situation of files but no line numbers, which seems a bit strange to me - well, at least for __va_list_tag, which isn't anymore associated with the file than with the line. Perhaps both should be addressed?

[MaskRay]

In D94735#2500129, @dblaikie wrote:

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Seems like this change might lead to a situation of files but no line numbers, which seems a bit strange to me - well, at least for __va_list_tag, which isn't anymore associated with the file than with the line. Perhaps both should be addressed?

__va_list_tag is synthesized by Clang CreateX86_64ABIBuiltinVaListDecl for __builtin_va_arg. I think giving it the filename of CurLoc is fine. Giving it the line of the CurLoc is arbitrary. CurLoc may be from a very unrelated thing.

Emm.. The title was wrong. I did not read 3319 if (T && !T->isForwardDecl()) carefully...

[dblaikie]

In D94735#2500132, @MaskRay wrote:

In D94735#2500129, @dblaikie wrote:

Ah, seems I was confused by the patch title - it looks like these values aren't used for FlagFwdDecl records - forward declarations bail out 3356, not using the file/line created on 3338/3339.

Seems like this change might lead to a situation of files but no line numbers, which seems a bit strange to me - well, at least for __va_list_tag, which isn't anymore associated with the file than with the line. Perhaps both should be addressed?

__va_list_tag is synthesized by Clang CreateX86_64ABIBuiltinVaListDecl for __builtin_va_arg. I think giving it the filename of CurLoc is fine. Giving it the line of the CurLoc is arbitrary. CurLoc may be from a very unrelated thing.

Emm.. The title was wrong. I did not read 3319 if (T && !T->isForwardDecl()) carefully...

What makes the file less arbitrary than the line? They seem similarly arbitrary to me.

[MaskRay]

Drop DW_AT_decl_file as well

[dblaikie]

Couple of optional pieces.

clang/lib/CodeGen/CGDebugInfo.cpp

3331–3339

Might be more readable, even though it provides the same behavior, with something like:

llvm::DIFile *DefUnit = nullptr;
unsigned Line = 0;
if (SourceLocation Loc = RD->getLocation(); Loc.isValid()) {
  DefUnit = getOrCreateFile(Loc);
  Line = getLineNumber(Loc);
}

clang/test/CodeGen/X86/x86_64-arguments.c

550

Perhaps CHECK-NOT would be a more explicit way to do that rather than relying on field ordering/the fact that 'size' comes after the file/line fields.

[MaskRay]

clang/lib/CodeGen/CGDebugInfo.cpp

3331–3339

init-statement in if is C++17, rejected by MSVC and warned by clang -std=c++14... [-Wc++17-extensions]

[dblaikie]

clang/lib/CodeGen/CGDebugInfo.cpp

3331–3339

Oh, right right! Well, nearby/similar code:

llvm::DIFile *DefUnit = nullptr;
unsigned Line = 0;
SourceLocation Loc = RD->getLocation(); 
if (Loc.isValid()) {
  DefUnit = getOrCreateFile(Loc);
  Line = getLineNumber(Loc);
}

[MaskRay]

comments

[protze.joachim]

@jdoerfert
This commit introduced the compile errors when OpenMP code is compiled with debug information. The symptom in llvm tests are the failing libarcher tests (which seem to be the only OpenMP test to be compiled with -g -fopenmp)
I have no idea how this patch is related to OpenMP codegen.

If anyone wants to have a look at a failing harbormaster example: https://reviews.llvm.org/harbormaster/unit/view/323409/

To reproduce the error manually:

llvm-project/build/./bin/clang llvm-project/openmp/tools/archer/tests/races/task-two.c -I llvm-project/build/projects/openmp/runtime/src -g -fopenmp

[protze.joachim]

Reverting this commit (on master) fixes the compiler errors.
This issue is tracked in https://bugs.llvm.org/show_bug.cgi?id=48953 , which is marked as blocking the 12 release.

Without any knowledge about the compiler backend or the Debug codegen: might it be possible, that this change impacts debug info for any #-line? Also #pragma omp ?

[dblaikie]

@MaskRay Sounds like this should be reverted (& that revert picked up by the 12 release branch) & some investigation can continue after that?

[MaskRay]

Perhaps we should just drop llvm/IR/Verifier.cpp:1075 (which is not tested):

// Note struct is allowed.
  if (N.getTag() == dwarf::DW_TAG_class_type ||
      N.getTag() == dwarf::DW_TAG_union_type) {
    AssertDI(N.getFile() && !N.getFile()->getFilename().empty(),
             "class/union requires a filename", &N, N.getFile());
  }

This patch is about a clang synthesized struct. clang/lib/CodeGen/CGOpenMPRuntime.cpp:3463 has a synthesized union (distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: <0x62b690>), which has no filename/line with this patch). It does not make sense to attach the current filename/line (which can be entirely irrelevant) to the synthesized union. @ABataev @jdoerfert ?

[jdoerfert]

In D94735#2546968, @dblaikie wrote:

This patch is about a clang synthesized struct. clang/lib/CodeGen/CGOpenMPRuntime.cpp:3463 has a synthesized union (distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: <0x62b690>), which has no filename/line with this patch). It does not make sense to attach the current filename/line (which can be entirely irrelevant) to the synthesized union. @ABataev @jdoerfert ?

There is no meaningful location for this generated type. Unclear if we should use a filename/line to specify that, e.g. "clang_synthesized:0", or allow it without. Either works for me.

[dblaikie]

In D94735#2547370, @jdoerfert wrote:

In D94735#2546968, @dblaikie wrote:

This patch is about a clang synthesized struct. clang/lib/CodeGen/CGOpenMPRuntime.cpp:3463 has a synthesized union (distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: <0x62b690>), which has no filename/line with this patch). It does not make sense to attach the current filename/line (which can be entirely irrelevant) to the synthesized union. @ABataev @jdoerfert ?

There is no meaningful location for this generated type. Unclear if we should use a filename/line to specify that, e.g. "clang_synthesized:0", or allow it without. Either works for me.

I'd vote in favour of having no location - lots of other types have no location (mostly builtin/fundamental types, admittedly) so should generally be fine. *fingers crossed*