This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Jan 14 2021, 5:31 PM.

Details

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

Diff Detail

Event Timeline

MaskRay created this revision.Jan 14 2021, 5:31 PM
MaskRay requested review of this revision.Jan 14 2021, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 5:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Jan 14 2021, 5:36 PM

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 added inline comments.Jan 14 2021, 6:20 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3334–3335

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 added inline comments.Jan 14 2021, 7:11 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3334–3335

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)

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 added a comment.EditedJan 14 2021, 8:51 PM

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...

MaskRay retitled this revision from CGDebugInfo: Drop unneeded line number for FlagFwdDecl declarations to CGDebugInfo: Drop unneeded line number for CreatedLimitedType.Jan 14 2021, 8:55 PM

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 updated this revision to Diff 316845.Jan 14 2021, 10:11 PM
MaskRay retitled this revision from CGDebugInfo: Drop unneeded line number for CreatedLimitedType to CGDebugInfo CreatedLimitedType: Drop file/line for RecordType with invalid location.
MaskRay edited the summary of this revision. (Show Details)

Drop DW_AT_decl_file as well

MaskRay edited the summary of this revision. (Show Details)Jan 14 2021, 10:12 PM
dblaikie accepted this revision.Jan 15 2021, 2:39 PM

Couple of optional pieces.

clang/lib/CodeGen/CGDebugInfo.cpp
3334–3342

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.

This revision is now accepted and ready to land.Jan 15 2021, 2:39 PM
MaskRay added inline comments.Jan 15 2021, 4:28 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3334–3342

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

dblaikie added inline comments.Jan 15 2021, 4:33 PM
clang/lib/CodeGen/CGDebugInfo.cpp
3334–3342

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);
}
This revision was landed with ongoing or failed builds.Jan 26 2021, 11:53 AM
This revision was automatically updated to reflect the committed changes.

@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 added a comment.EditedFeb 5 2021, 3:19 PM

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 ?

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

MaskRay added a subscriber: ABataev.EditedFeb 6 2021, 1:29 PM

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 ?

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.

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*