Page MenuHomePhabricator

[Verifier] Allow DW_TAG_class_type/DW_TAG_union_type to have no filename
ClosedPublic

Authored by MaskRay on Feb 6 2021, 1:37 PM.

Details

Summary

clang/lib/CodeGen/CGOpenMPRuntime.cpp synthesized union
(distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: <0x62b690>))
does not have meaningful filename/line number.

D94735 dropped the previously arbitrary and untested filename/line from the
union and caused a verifier error here.

This fixes check-libarcher failures.

Intended for main & release/12.x

Diff Detail

Event Timeline

MaskRay created this revision.Feb 6 2021, 1:37 PM
MaskRay requested review of this revision.Feb 6 2021, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2021, 1:37 PM
protze.joachim added a comment.EditedFeb 7 2021, 4:37 AM

I just compared the debug information generated for a build from 7630520ae + this patch with a build from 7630520ae + revert 189f3111. (7630520ae was just the top of main, when I did the bisecting, so I already had a complete build)

I diffed the generated .debug_info section for all binaries generated with check-libarcher. I used readelf -wi to extract the .debug_info section. The only difference is really just the DW_AT_producer attribute which is clear because the clang info contains the git hash.
So, at least for this small collection of OpenMP programs we don't loose debug information by 189f3111 + this patch.

This is all the information available for kmp_cmplrdata_t in the old and the new build:

<1><6c2e3>: Abbrev Number: 29 (DW_TAG_union_type)
   <6c2e4>   DW_AT_name        : (indirect string, offset: 0xddd4): kmp_cmplrdata_t
   <6c2e8>   DW_AT_byte_size   : 8

I suppose there is no good reason to require a file name. In other places we work around this by creating a file with the name "<compiler-generated>" but this doesn't add a lot of information. Do the various backends in AsmPrinter (DWARF+CodeView) accept null Files on these nodes?

aprantl accepted this revision.Feb 8 2021, 1:05 PM

In any case, I'm fine with this change, or with changing clang to create a special artificial file to stick the compiler-generated declarations into.

This revision is now accepted and ready to land.Feb 8 2021, 1:05 PM

I suppose there is no good reason to require a file name. In other places we work around this by creating a file with the name "<compiler-generated>" but this doesn't add a lot of information. Do the various backends in AsmPrinter (DWARF+CodeView) accept null Files on these nodes?

hmm - got any pointers to places where we create a file named "<compiler-generated>"? Wouldn't mind taking a look at those/maybe changing them to not have a name.

dblaikie accepted this revision.Feb 8 2021, 1:27 PM

In any case, I'm fine with this change, or with changing clang to create a special artificial file to stick the compiler-generated declarations into.

(oh, and I'm fine with it if/since Adrian is)

This revision was landed with ongoing or failed builds.Feb 8 2021, 1:31 PM
This revision was automatically updated to reflect the committed changes.