An upcoming patch will extend llvm-symbolizer to provide the source line
information for global variables. The goal is to move AddressSanitizer
off of internal debug info for symbolization onto the DWARF standard
(and doing a clean-up in the process). Currently, ASan reports the line
information for constant strings if a memory safety bug happens around
them. We want to keep this behaviour, so we need to emit debuginfo for
these variables as well.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This seems like it would significantly introduce debug info size for at least some kinds of code - have you done any size measurements of this change?
What does the resulting DWARF look like?
With -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_ENABLE_ASSERTIONS=On:
- Before the patch: clang was 1247264656 bytes
- After the patch: clang was 1270191696 bytes (or, a 1.84% increase)
What does the resulting DWARF look like?
Each string DI ends up looking like a regular global variable (i.e. a DW_TAG_variable entry in .debug_info):
$ llvm-dwarfdump --debug-info 0x00000032: DW_TAG_variable DW_AT_name (".str") DW_AT_type (0x0000003e ".str") DW_AT_decl_file ("/tmp/file.c") DW_AT_decl_line (3) DW_AT_location (DW_OP_addrx 0x1) DW_AT_linkage_name (".str")
(and obviously an entry in the .debug_addr referenced by the DW_AT_location at index 0x1)
section : increase in bytes for clang built with full debuginfo : %% of total binary size .debug_loclists 317782 0.0250% .debug_abbrev 88590 0.0070% .debug_info 10070834 0.7929% .debug_rnglists 143320 0.0113% .debug_str_offsets 1090516 0.0859% .debug_str 7789440 0.6132% .debug_addr 3611552 0.2843% .debug_line -187964 -0.0148% .debug_line_str 2043 0.0002%
That does seem a bit unfortunate/enough to be worth more examination.
What does the resulting DWARF look like?
Each string DI ends up looking like a regular global variable (i.e. a DW_TAG_variable entry in .debug_info):
$ llvm-dwarfdump --debug-info 0x00000032: DW_TAG_variable DW_AT_name (".str") DW_AT_type (0x0000003e ".str") DW_AT_decl_file ("/tmp/file.c") DW_AT_decl_line (3) DW_AT_location (DW_OP_addrx 0x1) DW_AT_linkage_name (".str")(and obviously an entry in the .debug_addr referenced by the DW_AT_location at index 0x1)
What's the DW_AT_type referring to? (why is there a type called ".str"?)
For your particular purposes, it seems like the name/type/linkage name could all be omitted & that might be OK, since these entities aren't actually named in the source?
What's the DW_AT_type referring to? (why is there a type called ".str"?)
I'd expect it to point to a base type, in this case character string. A typeless variable with a location would be pretty weird.
But I agree the name/linkage_name would be unnecessary.
I've updated the patch to:
- Omit the linkage name,
- Omit the display name (including associated changes to allow this), and
- Used the char[SIZE] type.
#3 could be optimised for file size further by changing to a const char* type, but I:
- Can't see an obvious way to synthesize a const char* QualType to pass to createGlobalVariableExpression, and
- Think it might actually be useful to have the correct type here (as it now also describes the size of the variable).
Here's the new dwarfdump:
0x00000065: DW_TAG_variable DW_AT_type (0x0000006f "char [11]") DW_AT_decl_file ("/tmp/file.c") DW_AT_decl_line (4) DW_AT_location (DW_OP_addrx 0x3) 0x0000006f: DW_TAG_array_type DW_AT_type (0x00000048 "char") 0x00000074: DW_TAG_subrange_type DW_AT_type (0x0000004c "__ARRAY_SIZE_TYPE__") DW_AT_count (0x0b) 0x0000007a: NULL
Apparently with -DCMAKE_BUILD_TYPE='RelWithDebInfo', bin/clang-15 went from 1373029152B to 1373030960B (~0%), which seems suspiciously low to me. I double checked the builds twice, and they seem to be right...
Confirmed again with -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_ENABLE_ASSERTIONS=On:
1510883864 (new) - 1510881800 (old) = 2064 bytes.
Yeah, as I drove away from my desk I realized my mistake. Will get new accurate numbers.
1263559896 (new) - 1248280328 (old) = 15279568 B (1.22% increase). Seems more reasonable to me.
I just did an experiment with the following patch that punts everything to const char* types to try and get better folding onto a single type:
The change didn't make a significant difference, with the clang binary size of 1263433512 (126384B reduction, or 1.224% -> 1.214%). I think it's better to keep the extra size diagnostic.
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp index f82c0e357bc3..175a5fec2989 100644 --- a/clang/lib/CodeGen/CGDebugInfo.cpp +++ b/clang/lib/CodeGen/CGDebugInfo.cpp @@ -5454,11 +5454,23 @@ void CGDebugInfo::AddStringLiteralDebugInfo(llvm::GlobalVariable *GV, if (!PLoc.isValid()) return; + // Official type of the StringLiteral is `char[N]`. This means for each + // compilation unit, we have multiple DW_*_type entries, one set for each size + // of constant string. This level of precision isn't necessary, so simply + // synthesize a `const char*` type so it can be reused across all constant + // strings. + const auto &Context = CGM.getContext(); + QualType Type = Context.CharTy; + Type.addConst(); + Type = Context.getPointerType(Type); + + // Similar to above, elide the name variables (as there isn't really anything + // useful we can add) to save binary size. llvm::DIFile *File = getOrCreateFile(Loc); llvm::DIGlobalVariableExpression *Debug = DBuilder.createGlobalVariableExpression( nullptr, StringRef(), StringRef(), getOrCreateFile(Loc), - getLineNumber(Loc), getOrCreateType(S->getType(), File), true); + getLineNumber(Loc), getOrCreateType(Type, File), true); GV->addDebugInfo(Debug); }
Size still seems moderately concerning, but might be acceptable.
Got a summary of what the DWARF looks like now (without names)? (maybe there's something else we can strip/optimize)
& how many of these descriptions get added to the debug info?
Numbers for Split DWARF may be helpful too - given this'll add an extra address/relocation for every string literal, it might make object size (specifically unlinked object size where relocations are expensive/plentiful) significantly larger in problematic ways.
clang/test/CodeGen/debug-info-variables.c | ||
---|---|---|
2 | Presumably named variables are already tested elsewhere - so I probably wouldn't test them here, and would give this file a more specific name related to string literals. Alternatively/perhaps the other thing to do would be to find some test that already covers global variable debug info, and test string literals there? |
summary of DWARF:
& how many of these descriptions get added to the debug info?
afaict, there is now:
1x .debug_addr entry for each string
1x. debug_info DW_TAG_variable for each string
1x. DW_TAG_array_type + DW_TAG_subrange_type for each unique sizeof(string)
i tried to measure if there's other bits laying around that could be optimised. i thought briefly about diffing the llvm-dwarfdump for the before/after for clang, but as the dumpfiles reached 20gb, rethought that decision. the dwarfdump for the clang/test/CodeGen/debug-info-variables.c dwo is below.
Numbers for Split DWARF may be helpful too - given this'll add an extra address/relocation for every string literal, it might make object size (specifically unlinked object size where relocations are expensive/plentiful) significantly larger in problematic ways.
sorry, i don't understand why split-dwarf means this requires an additional relocation (i'm not really sure what split-dwarf is outside of just putting the dwarf in a separate file, but don't see why that would change relocations). i made a quick dwarfdump diff on clang/test/CodeGen/debug-info-variables.c (with split-dwarf):
sections old:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 2] .debug_str.dwo PROGBITS 0000000000000000 000040 0000eb 01 MSE 0 0 1 [ 3] .debug_str_offsets.dwo PROGBITS 0000000000000000 00012b 00002c 00 E 0 0 1 [ 4] .debug_info.dwo PROGBITS 0000000000000000 000157 000077 00 E 0 0 1 [ 5] .debug_abbrev.dwo PROGBITS 0000000000000000 0001ce 000091 00 E 0 0 1
sections new:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 3] .debug_str.dwo PROGBITS 0000000000000000 000078 0000ff 01 MSE 0 0 1 [ 2] .debug_str_offsets.dwo PROGBITS 0000000000000000 000040 000038 00 E 0 0 1 [ 4] .debug_info.dwo PROGBITS 0000000000000000 000177 000092 00 E 0 0 1 [ 5] .debug_abbrev.dwo PROGBITS 0000000000000000 000209 0000aa 00 E 0 0 1
so .debug_string += 0x14, .debug_str_offsets += 0xc, .debug_info += 0x1b and .debug_abbrev += 0x19.
as before, the DW_TAG_array_type + DW_TAG_subrange_type would be amortised across strings with the same size.
unfortunately, i don't see any further places to optimise (except for the full const char* amortization, which as in a previous comment, didn't make much of an improvement for the entire clang binary)
diff --git a/tmp/dwo/dwo-dump b/dwo-dump index e0fdd77..6415086 100644 --- a/tmp/dwo/dwo-dump +++ b/dwo-dump @@ -3,14 +3,13 @@ debug-info-variables.dwo: file format elf64-x86-64 .debug_abbrev.dwo contents: Abbrev table for offset: 0x00000000 [1] DW_TAG_compile_unit DW_CHILDREN_yes - DW_AT_producer DW_FORM_GNU_str_index + DW_AT_producer DW_FORM_strx1 DW_AT_language DW_FORM_data2 - DW_AT_name DW_FORM_GNU_str_index - DW_AT_GNU_dwo_name DW_FORM_GNU_str_index - DW_AT_GNU_dwo_id DW_FORM_data8 + DW_AT_name DW_FORM_strx1 + DW_AT_dwo_name DW_FORM_strx1 [2] DW_TAG_variable DW_CHILDREN_no - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_type DW_FORM_ref4 DW_AT_external DW_FORM_flag_present DW_AT_decl_file DW_FORM_data1 @@ -18,155 +17,194 @@ Abbrev table for offset: 0x00000000 DW_AT_location DW_FORM_exprloc [3] DW_TAG_base_type DW_CHILDREN_no - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_encoding DW_FORM_data1 DW_AT_byte_size DW_FORM_data1 -[4] DW_TAG_subprogram DW_CHILDREN_no - DW_AT_low_pc DW_FORM_GNU_addr_index +[4] DW_TAG_variable DW_CHILDREN_no + DW_AT_type DW_FORM_ref4 + DW_AT_decl_file DW_FORM_data1 + DW_AT_decl_line DW_FORM_data1 + DW_AT_location DW_FORM_exprloc + +[5] DW_TAG_array_type DW_CHILDREN_yes + DW_AT_type DW_FORM_ref4 + +[6] DW_TAG_subrange_type DW_CHILDREN_no + DW_AT_type DW_FORM_ref4 + DW_AT_count DW_FORM_data1 + +[7] DW_TAG_base_type DW_CHILDREN_no + DW_AT_name DW_FORM_strx1 + DW_AT_byte_size DW_FORM_data1 + DW_AT_encoding DW_FORM_data1 + +[8] DW_TAG_subprogram DW_CHILDREN_no + DW_AT_low_pc DW_FORM_addrx DW_AT_high_pc DW_FORM_data4 DW_AT_frame_base DW_FORM_exprloc - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_decl_file DW_FORM_data1 DW_AT_decl_line DW_FORM_data1 DW_AT_type DW_FORM_ref4 DW_AT_external DW_FORM_flag_present -[5] DW_TAG_subprogram DW_CHILDREN_yes - DW_AT_low_pc DW_FORM_GNU_addr_index +[9] DW_TAG_subprogram DW_CHILDREN_yes + DW_AT_low_pc DW_FORM_addrx DW_AT_high_pc DW_FORM_data4 DW_AT_frame_base DW_FORM_exprloc - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_decl_file DW_FORM_data1 DW_AT_decl_line DW_FORM_data1 DW_AT_prototyped DW_FORM_flag_present DW_AT_type DW_FORM_ref4 DW_AT_external DW_FORM_flag_present -[6] DW_TAG_formal_parameter DW_CHILDREN_no +[10] DW_TAG_formal_parameter DW_CHILDREN_no DW_AT_location DW_FORM_exprloc - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_decl_file DW_FORM_data1 DW_AT_decl_line DW_FORM_data1 DW_AT_type DW_FORM_ref4 -[7] DW_TAG_variable DW_CHILDREN_no +[11] DW_TAG_variable DW_CHILDREN_no DW_AT_location DW_FORM_exprloc - DW_AT_name DW_FORM_GNU_str_index + DW_AT_name DW_FORM_strx1 DW_AT_decl_file DW_FORM_data1 DW_AT_decl_line DW_FORM_data1 DW_AT_type DW_FORM_ref4 -[8] DW_TAG_pointer_type DW_CHILDREN_no +[12] DW_TAG_pointer_type DW_CHILDREN_no DW_AT_type DW_FORM_ref4 -[9] DW_TAG_const_type DW_CHILDREN_no +[13] DW_TAG_const_type DW_CHILDREN_no DW_AT_type DW_FORM_ref4 .debug_info.dwo contents: -0x00000000: Compile Unit: length = 0x00000073, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000077) +0x00000000: Compile Unit: length = 0x0000008e, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset = 0x0000, addr_size = 0x08, DWO_id = 0xa7a0aceb112fa998 (next unit at 0x00000092) -0x0000000b: DW_TAG_compile_unit - DW_AT_producer ("clang version 14.0.0 (https://github.com/llvm/llvm-project.git 5357a98c823a5262814e269db265b5d8e1f2c4f2)") +0x00000014: DW_TAG_compile_unit + DW_AT_producer ("clang version 15.0.0 (https://github.com/llvm/llvm-project.git 4c7fff8247ec5485701d4e04ea45b9fe399e1c5a)") DW_AT_language (DW_LANG_C99) DW_AT_name ("/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/debug-info-variables.c") - DW_AT_GNU_dwo_name ("debug-info-variables.dwo") - DW_AT_GNU_dwo_id (0x02255219cf8b78f3) + DW_AT_dwo_name ("debug-info-variables.dwo") -0x00000019: DW_TAG_variable +0x0000001a: DW_TAG_variable DW_AT_name ("global") - DW_AT_type (0x00000024 "int") + DW_AT_type (0x00000025 "int") DW_AT_external (true) DW_AT_decl_file (0x01) DW_AT_decl_line (4) - DW_AT_location (DW_OP_GNU_addr_index 0x0) + DW_AT_location (DW_OP_addrx 0x0) -0x00000024: DW_TAG_base_type +0x00000025: DW_TAG_base_type DW_AT_name ("int") DW_AT_encoding (DW_ATE_signed) DW_AT_byte_size (0x04) -0x00000028: DW_TAG_subprogram - DW_AT_low_pc (indexed (00000001) address = <unresolved>) +0x00000029: DW_TAG_variable + DW_AT_type (0x00000033 "char [11]") + DW_AT_decl_file (0x01) + DW_AT_decl_line (8) + DW_AT_location (DW_OP_addrx 0x1) + +0x00000033: DW_TAG_array_type + DW_AT_type (0x0000003f "char") + +0x00000038: DW_TAG_subrange_type + DW_AT_type (0x00000043 "__ARRAY_SIZE_TYPE__") + DW_AT_count (0x0b) + +0x0000003e: NULL + +0x0000003f: DW_TAG_base_type + DW_AT_name ("char") + DW_AT_encoding (DW_ATE_signed_char) + DW_AT_byte_size (0x01) + +0x00000043: DW_TAG_base_type + DW_AT_name ("__ARRAY_SIZE_TYPE__") + DW_AT_byte_size (0x08) + DW_AT_encoding (DW_ATE_unsigned) + +0x00000047: DW_TAG_subprogram + DW_AT_low_pc (indexed (00000002) address = <unresolved>) DW_AT_high_pc (0x0000000d) DW_AT_frame_base (DW_OP_reg6 RBP) DW_AT_name ("s") DW_AT_decl_file (0x01) DW_AT_decl_line (7) - DW_AT_type (0x00000068 "const char *") + DW_AT_type (0x00000087 "const char *") DW_AT_external (true) -0x00000037: DW_TAG_subprogram - DW_AT_low_pc (indexed (00000002) address = <unresolved>) +0x00000056: DW_TAG_subprogram + DW_AT_low_pc (indexed (00000003) address = <unresolved>) DW_AT_high_pc (0x00000018) DW_AT_frame_base (DW_OP_reg6 RBP) DW_AT_name ("sum") DW_AT_decl_file (0x01) DW_AT_decl_line (14) DW_AT_prototyped (true) - DW_AT_type (0x00000024 "int") + DW_AT_type (0x00000025 "int") DW_AT_external (true) -0x00000046: DW_TAG_formal_parameter +0x00000065: DW_TAG_formal_parameter DW_AT_location (DW_OP_fbreg -4) DW_AT_name ("p") DW_AT_decl_file (0x01) DW_AT_decl_line (14) - DW_AT_type (0x00000024 "int") + DW_AT_type (0x00000025 "int") -0x00000051: DW_TAG_formal_parameter +0x00000070: DW_TAG_formal_parameter DW_AT_location (DW_OP_fbreg -8) DW_AT_name ("q") DW_AT_decl_file (0x01) DW_AT_decl_line (14) - DW_AT_type (0x00000024 "int") + DW_AT_type (0x00000025 "int") -0x0000005c: DW_TAG_variable +0x0000007b: DW_TAG_variable DW_AT_location (DW_OP_fbreg -12) DW_AT_name ("r") DW_AT_decl_file (0x01) DW_AT_decl_line (15) - DW_AT_type (0x00000024 "int") - -0x00000067: NULL + DW_AT_type (0x00000025 "int") -0x00000068: DW_TAG_pointer_type - DW_AT_type (0x0000006d "const char") +0x00000086: NULL -0x0000006d: DW_TAG_const_type - DW_AT_type (0x00000072 "char") +0x00000087: DW_TAG_pointer_type + DW_AT_type (0x0000008c "const char") -0x00000072: DW_TAG_base_type - DW_AT_name ("char") - DW_AT_encoding (DW_ATE_signed_char) - DW_AT_byte_size (0x01) +0x0000008c: DW_TAG_const_type + DW_AT_type (0x0000003f "char") -0x00000076: NULL +0x00000091: NULL .debug_str.dwo contents: 0x00000000: "global" 0x00000007: "int" -0x0000000b: "s" -0x0000000d: "char" -0x00000012: "sum" -0x00000016: "p" -0x00000018: "q" -0x0000001a: "r" -0x0000001c: "clang version 14.0.0 (https://github.com/llvm/llvm-project.git 5357a98c823a5262814e269db265b5d8e1f2c4f2)" -0x00000085: "/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/debug-info-variables.c" -0x000000d2: "debug-info-variables.dwo" +0x0000000b: "char" +0x00000010: "__ARRAY_SIZE_TYPE__" +0x00000024: "s" +0x00000026: "sum" +0x0000002a: "p" +0x0000002c: "q" +0x0000002e: "r" +0x00000030: "clang version 15.0.0 (https://github.com/llvm/llvm-project.git 4c7fff8247ec5485701d4e04ea45b9fe399e1c5a)" +0x00000099: "/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/debug-info-variables.c" +0x000000e6: "debug-info-variables.dwo" .debug_str_offsets.dwo contents: -0x00000000: Contribution size = 44, Format = DWARF32, Version = 4 -0x00000000: 00000000 "global" -0x00000004: 00000007 "int" -0x00000008: 0000000b "s" -0x0000000c: 0000000d "char" -0x00000010: 00000012 "sum" -0x00000014: 00000016 "p" -0x00000018: 00000018 "q" -0x0000001c: 0000001a "r" -0x00000020: 0000001c "clang version 14.0.0 (https://github.com/llvm/llvm-project.git 5357a98c823a5262814e269db265b5d8e1f2c4f2)" -0x00000024: 00000085 "/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/debug-info-variables.c" -0x00000028: 000000d2 "debug-info-variables.dwo" +0x00000000: Contribution size = 52, Format = DWARF32, Version = 5 +0x00000008: 00000000 "global" +0x0000000c: 00000007 "int" +0x00000010: 0000000b "char" +0x00000014: 00000010 "__ARRAY_SIZE_TYPE__" +0x00000018: 00000024 "s" +0x0000001c: 00000026 "sum" +0x00000020: 0000002a "p" +0x00000024: 0000002c "q" +0x00000028: 0000002e "r" +0x0000002c: 00000030 "clang version 15.0.0 (https://github.com/llvm/llvm-project.git 4c7fff8247ec5485701d4e04ea45b9fe399e1c5a)" +0x00000030: 00000099 "/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/debug-info-variables.c" +0x00000034: 000000e6 "debug-info-variables.dwo"
clang/test/CodeGen/debug-info-variables.c | ||
---|---|---|
2 | I found debug-info-global-constant.c and debug-info-static.c, but neither of them test DILocalVariables or strings. This test was cargo-culted from debug-info-gline-tables-only.c which does the inverse :). I was hoping that this test would become the main "simple global variables" test :) |
& you ran an experiment where the type was omitted, but it didn't save much space?
i tried to measure if there's other bits laying around that could be optimised. i thought briefly about diffing the llvm-dwarfdump for the before/after for clang, but as the dumpfiles reached 20gb, rethought that decision. the dwarfdump for the clang/test/CodeGen/debug-info-variables.c dwo is below.
Yeah, diffing DWARF isn't super practical with all the offsets, etc.
Numbers for Split DWARF may be helpful too - given this'll add an extra address/relocation for every string literal, it might make object size (specifically unlinked object size where relocations are expensive/plentiful) significantly larger in problematic ways.
sorry, i don't understand why split-dwarf means this requires an additional relocation (i'm not really sure what split-dwarf is outside of just putting the dwarf in a separate file, but don't see why that would change relocations).
Sorry, to expound more: Split DWARF is intended to reduce the size of object files processed by the linker, and the resulting linked binary. Doing things like adding more pure type information which only goes in the .dwo file has no impact on .o/executable size - but this change adds another relocation for every string literal in the binary, which does grow the debug info that remains in the .o file (the .debug_addr table in particular) and relocations in particular are quite expensive (since they aren't compressed by -gz and take 3x the bytes of the actual address entry)
We should probably do some measurements inside google on these things, since the object size metrics are quite important to linkability in bazel/blaze (go/binary-size-analysis internally has some info - happy to help with measurements, etc).
i made a quick dwarfdump diff on clang/test/CodeGen/debug-info-variables.c (with split-dwarf):
sections old:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 2] .debug_str.dwo PROGBITS 0000000000000000 000040 0000eb 01 MSE 0 0 1 [ 3] .debug_str_offsets.dwo PROGBITS 0000000000000000 00012b 00002c 00 E 0 0 1 [ 4] .debug_info.dwo PROGBITS 0000000000000000 000157 000077 00 E 0 0 1 [ 5] .debug_abbrev.dwo PROGBITS 0000000000000000 0001ce 000091 00 E 0 0 1sections new:
[Nr] Name Type Address Off Size ES Flg Lk Inf Al [ 3] .debug_str.dwo PROGBITS 0000000000000000 000078 0000ff 01 MSE 0 0 1 [ 2] .debug_str_offsets.dwo PROGBITS 0000000000000000 000040 000038 00 E 0 0 1 [ 4] .debug_info.dwo PROGBITS 0000000000000000 000177 000092 00 E 0 0 1 [ 5] .debug_abbrev.dwo PROGBITS 0000000000000000 000209 0000aa 00 E 0 0 1so .debug_string += 0x14, .debug_str_offsets += 0xc, .debug_info += 0x1b and .debug_abbrev += 0x19.
This data, but for Clang, might be informative - for this small sample it's hard to separate the constant overheads from the linear ones. (eg: the debug_str growth I'd expect to be constant (just the name of the char type and the name of the size type), but the debug_info growth is presumably not constant)
Tried this patch on some internal targets and metrics and it seems actually quite reasonable.
Tested with split DWARF, on Linux, with compressed debug info in .o/.dwo files, uncompressed in exe/dwp files. With -O0 and -O3, roughly speaking. Testing Clang as well as testing a large Google binary only at -O3:
Clang:
-O0:
.o: +0.44%
.dwo: +1.55%
exe: 0.65%
dwp: 1.42%
-O3:
.o: 0.90%
.dwo: 0.26%
exe: 0.33%
dwp: 0.41%
Large Google Binary -O3:
.o: 0.32%
.dwo: 0.16%
exe: 0.19%
dwp: 0.22%
These are % of total size, not % of debug info contribution - so it's growing the debug info by more than that, but the total impact doesn't seem to be too bad.
@rnk @aprantl @probinson: this looks plausible to me to take as a general change to the emitted DWARF, but I'm open to ideas/suggestions if folks find this to be too much growth for not enough benefit & that it should be behind a flag?
This seems reasonable, but I worry about the consequences of creating lots of unnamed global variables. What will gdb do with so many unnamed globals? What will the PDB linker do with all these unnamed globals? I can't answer these questions, and you're welcome to try and find out, but I predict there will be problems, so just be aware of that possibility.
Looking at the S_GDATA32 record format, there is no place for the file & line info:
https://github.com/microsoft/microsoft-pdb/blob/master/include/cvinfo.h#L3629
It's just type info & name, really, so there's not much point in creating these when emitting codeview. It's probably best to filter these unnamed globals out in AsmPrinter/CodeViewDebug.cpp rather than changing the IR clang generates.
Not sure about PDB. I did run a quick test with gdb, and very unscientifically, didn't notice any difference in usability or errors between pre- and post-this patch on a clang invocation under gdb.
Looking at the S_GDATA32 record format, there is no place for the file & line info:
https://github.com/microsoft/microsoft-pdb/blob/master/include/cvinfo.h#L3629
It's just type info & name, really, so there's not much point in creating these when emitting codeview. It's probably best to filter these unnamed globals out in AsmPrinter/CodeViewDebug.cpp rather than changing the IR clang generates.
Sorry, I know pretty nothing about PDB. Are you thinking that these DIGlobalVariables should be filtered out by this patch when outputting PDB, or a subsequent patch? I don't even have a Windows machine, so I wouldn't be at all confident in writing such a patch.
That's good enough for me. Maybe there will be problems down the road, but the best way to find out is probably to land the change.
Sorry, I know pretty nothing about PDB. Are you thinking that these DIGlobalVariables should be filtered out by this patch when outputting PDB, or a subsequent patch? I don't even have a Windows machine, so I wouldn't be at all confident in writing such a patch.
That's fine, a Windows machine shouldn't be necessary. This patch has to change the AsmPrinter for DWARF, so it makes sense that it should do the same for CodeView. I think it should be sufficient to continue this loop when a global variable display name is empty:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp#L3175
It will need a test, and the tests at llvm/test/DebugInfo/COFF/global* should be good templates. Basically, have a file with two globals, one named and unnamed, and use FileCheck to confirm that there is only one GDATA record.
Thanks for the pointer to the test. That was very helpful in figuring out how to invoke clang to produce CodeView/COFF. Funnily enough, it found a bug where I didn't realise I had to relax the invariant that a DIGlobalVariable needs a name in the LL parser.
I wonder whether it's feasible to re-use FunctionLineTable to encode line:col for data symbols. A hack for sure, and left as an exercise for the reader.
Looks good to me. I generally agree with Adrian's suggestions.
llvm/test/DebugInfo/COFF/global-no-strings.ll | ||
---|---|---|
2 | test lgtm, thanks! |
Seems good - if folks find the growth too much, or it creates weird perf issues otherwise, we can discuss that if/when it comes up.
Hi, we're seeing a breakage in Fuchsia's clang CI for x64 windows that I think is related to this patch.
We see a test failure in Clang :: CodeGen/debug-info-variables.c, which this patch modifies.
If this will be hard to fix, would you mind reverting until a fix is ready?
Why, on Fuchsia, on WIndows, the DIs are in the order q -> p -> r is beyond me (instead of the file order and my order p -> q -> r. Looking into it.
Fix the fuchsia windows bot by making the DILocalVariables in the test order-independent.
We noticed when building Fuchsia using ToT LLVM that CGo has trouble with the new changes to DWARF introduced in this patch. It appears as if CGo is confused because it found a DW_TAG_variable that doesn't have a name, which is allowed by the DWARF standard.
This patch seems like a good change to me and is well thought out and tested, but I think this may cause issues in the wider ecosystem as people interact with it. It may simply be a problem w/ Go, and a bug fix there is all that is necessary, but there is also the chance that other compilers and toolchains will have similar issues.
I want to be clear that I don't necessarily think a revert is in order, but we should take the opportunity to determine the right course of action.
Thanks for the heads up. A possible thought is to send an announcement of "this happened" to llvm-dev, but I'm not sure that would reach the intended audience, as LLVM tooling is effectively "fixed up" inside this patch, and it's more the consumers of LLVM that may need changes.
Might be worth a note in the Release Notes given that it's at least caused issues with one DWARF consumer.
This patch appears to break CUDA compilation: https://github.com/llvm/llvm-project/issues/58491
We apparently emit the symbol with a character (.) that can't be used on the target. Normally such characters get renamed/escaped, as you can see in the generated symbol name itself.
Search for .b64 __func__._Z10foo_kernelv in the dwarf output.
https://godbolt.org/z/8h4rGn1Ka
I would appreciate a comment explaining the motivation behind doing this here.