Page MenuHomePhabricator

[DebugInfo][SROA] Correct debug info for global variables spanning multiple fragments in case of SROA
Needs ReviewPublic

Authored by alok on Nov 30 2022, 2:21 AM.

Details

Summary

The function transferSRADebugInfo is modified to include missing cases.

The existing handling produced crash for test case (attached with patch).

Please consider below IR

%struct.BSS3 = type <{ [24 x i8] }>
@.BSS3 = internal unnamed_addr global %struct.BSS3 zeroinitializer, align 32, !dbg !0, !dbg !7, !dbg !29

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "bar1", scope: !2, file: !4, type: !12, isLocal: true, isDefinition: true)
!12 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, size: 96, align: 32, elements: !14)

In SROA @.BSS3 is broken into @.BSS3.0, @.BSS3.1, @.BSS3.2

@.BSS3.0 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg !0, !dbg !24
@.BSS3.1 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg !26, !dbg !27
@.BSS3.2 = internal unnamed_addr global double 0.000000e+00, align 16, !dbg !28

Since original memory chunk is broken into three (each of size 64bits), the variable "bar1" (of size 96=64+32) spreads across @.BSS3.0 (completely) and @.BSS3.1 (partially). So below DebugInfo should be generated.

@.BSS3.0 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg !0, !dbg !24
@.BSS3.1 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg !25, !dbg !26, !dbg !27
@.BSS3.2 = internal unnamed_addr global double 0.000000e+00, align 16, !dbg !28

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression(DW_OP_LLVM_fragment, 0, 64))
!1 = distinct !DIGlobalVariable(name: "bar1", scope: !2, file: !4, type: !13, isLocal: true, isDefinition: true)
!25 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression(DW_OP_LLVM_fragment, 64, 32))

But currently it was wrongly generating DW_OP_LLVM_fragment DIExpression with incorrect second argument (64 in place of 32).

fragment is larger than or outside of variable
!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression(DW_OP_LLVM_fragment, 0, 64))
!26 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression(DW_OP_LLVM_fragment, 64, 64))

Later the verification fails as variable size is less than fragment. Which leads a crash.

Breakpoint 2, (anonymous namespace)::Verifier::verifyFragmentExpression<llvm::DIGlobalVariableExpression const> (this=0x7fffffffb790, V=..., Fragment=...,
    Desc=0x555555712450) at /tmp/llvm-project/llvm/lib/IR/Verifier.cpp:6135
6135   CheckDI(FragSize + FragOffset <= *VarSize,
6136           "fragment is larger than or outside of variable", Desc, &V);

Now the function transferSRADebugInfo is modified to handle such cases.

Diff Detail

Unit TestsFailed

TimeTest
60,060 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
60,060 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,040 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
60,030 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

Event Timeline

alok created this revision.Nov 30 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald Transcript
alok requested review of this revision.Nov 30 2022, 2:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 2:21 AM

Before digging into the code, I'm not sure I understand the failing IR completely -- for the segment:

%struct.BSS3 = type <{ [24 x i8] }>
@.BSS3 = internal unnamed_addr global %struct.BSS3 zeroinitializer, align 32, !dbg !0, !dbg !7, !dbg !29

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "bar1", scope: !2, file: !4, type: !12, isLocal: true, isDefinition: true)
!12 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, size: 96, align: 32, elements: !14)

Isn't the debug-info type for the global variable wrong? 24 bytes (broken into 3 doubles by SROA) is 192 bits, which is surely the cause of the "fragment is larger than or outside of variable" when SROA splits the 24 bytes into 3 doubles.

alok added a comment.Nov 30 2022, 7:17 AM

Before digging into the code, I'm not sure I understand the failing IR completely -- for the segment:

%struct.BSS3 = type <{ [24 x i8] }>
@.BSS3 = internal unnamed_addr global %struct.BSS3 zeroinitializer, align 32, !dbg !0, !dbg !7, !dbg !29

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "bar1", scope: !2, file: !4, type: !12, isLocal: true, isDefinition: true)
!12 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, size: 96, align: 32, elements: !14)

Isn't the debug-info type for the global variable wrong? 24 bytes (broken into 3 doubles by SROA) is 192 bits, which is surely the cause of the "fragment is larger than or outside of variable" when SROA splits the 24 bytes into 3 doubles.

@.BSS3 (size=192) is shared by three different variables. (Please consider llvm/test/DebugInfo/X86/global-sra-struct-part-overlap-segment.ll of this patch).
"bar1" (size=96, offset=0),
"rvar" (size=192, offset=0) and
"ivar" (size=64, offset=32)
"rvar" is split by SROA, "bar1" adjusts itself as a side effect.

aprantl added inline comments.Nov 30 2022, 9:33 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
431

Why initialize to zero and then again here?

437–460

tbh, I found the original comment easier to understand

439

Aren't these two conditions trivially true because of the check above?

alok updated this revision to Diff 479016.Nov 30 2022, 10:29 AM

Re-based and included comments from @aprantl.

alok marked an inline comment as done.Nov 30 2022, 10:35 AM
alok added inline comments.
llvm/lib/Transforms/IPO/GlobalOpt.cpp
431

Why initialize to zero and then again here?

Thanks for pointing this out. I have now removed initialize with zero.

437–460

tbh, I found the original comment easier to understand

Thanks, the comment is updated now.

439

Aren't these two conditions trivially true because of the check above?

Checks at line 427 and 433 assure that current variable does not overlap with current fragment.
Current condition further checks whether current variable overlaps only (fits) current fragment, failing check is when current variable overlaps other fragments also and needs fragment expression.

alok marked an inline comment as done.Dec 4 2022, 11:08 PM
alok updated this revision to Diff 480829.Dec 7 2022, 3:03 AM

Re-based.

alok added a comment.Dec 7 2022, 4:15 AM

Hi @aprantl , do you have more comments ?

alok updated this revision to Diff 483173.Dec 15 2022, 7:10 AM

Re-based.

I have some low-level comments inline and a high-level question: Is there a common algorithm that could be factored out from SROA.cpp and reused here?

llvm/lib/Transforms/IPO/GlobalOpt.cpp
451

could we replace this else with a continue/return inside the previous block?

460

Can you clean up the logic here such that we don't create a new empty expression, if we overwrite it immediately on line 463?