Fix undefined behavior in Dwarf.

[linzj]

Summary

Caused by field PrevCU is not initialized.
llvm::DwarfCompileUnit::addRange () at ../lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:276
llvm::DwarfDebug::endFunctionImpl () at ../lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1586
llvm::DebugHandlerBase::endFunction () at ../lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:319
llvm::AsmPrinter::EmitFunctionBody () at ../lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1230
llvm::ARMAsmPrinter::runOnMachineFunction () at ../lib/Target/ARM/ARMAsmPrinter.cpp:161

[linzj]

I am not a commiter, please commit this patch for me.

[dblaikie]

Is this UB codepath reached by existing test cases running in check-llvm? Or was it found on some other testing?

[linzj]

In D81631#2093513, @dblaikie wrote:

Is this UB codepath reached by existing test cases running in check-llvm? Or was it found on some other testing?

No. I ran my compiler which is based on LLVM with valgrind. Then this warning was popping everywhere.

[dblaikie]

In D81631#2094574, @linzj wrote:

In D81631#2093513, @dblaikie wrote:

Is this UB codepath reached by existing test cases running in check-llvm? Or was it found on some other testing?

No. I ran my compiler which is based on LLVM with valgrind. Then this warning was popping everywhere.

Please ensure/add a test case to the LLVM regression tests that demonstrates the codepath to ensure it has some coverage/regression testing. (if this doesn't reproduce with Clang's IR/LLVM's regression tests, it's possible your compiler is producing some novel IR that we're not testing/haven't thought about)

[linzj]

Add testcase.

[dblaikie]

Could you include a comment in the test case explaining how this IR differs from all the IR clang usually generates? (in general what's interesting about this IR)

Also - remove the "valgrind" usage from that llc invocation (test machines might not have valgrind, etc - does UBSan/ASan/MSan catch this? There are buildbots that'll run the tests with sanitizers like those enabled & it'd be good if the test could catch a regression at least on those buildbots) - and include some testing of the behavior of llc (ie: what behavior do we get instead of UB? Presumably there's some specific behavior we're expecting beyond "anything other than crashing")

[linzj]

In D81631#2101784, @dblaikie wrote:

Could you include a comment in the test case explaining how this IR differs from all the IR clang usually generates? (in general what's interesting about this IR)

Also - remove the "valgrind" usage from that llc invocation (test machines might not have valgrind, etc - does UBSan/ASan/MSan catch this? There are buildbots that'll run the tests with sanitizers like those enabled & it'd be good if the test could catch a regression at least on those buildbots) - and include some testing of the behavior of llc (ie: what behavior do we get instead of UB? Presumably there's some specific behavior we're expecting beyond "anything other than crashing")

It just a simple function complied by clang with -g. No modification.

I don't known how to write a test case that tell the try not to build the LLC with ubsan, could you give me an example?

[linzj]

In D81631#2101784, @dblaikie wrote:

Could you include a comment in the test case explaining how this IR differs from all the IR clang usually generates? (in general what's interesting about this IR)

Also - remove the "valgrind" usage from that llc invocation (test machines might not have valgrind, etc - does UBSan/ASan/MSan catch this? There are buildbots that'll run the tests with sanitizers like those enabled & it'd be good if the test could catch a regression at least on those buildbots) - and include some testing of the behavior of llc (ie: what behavior do we get instead of UB? Presumably there's some specific behavior we're expecting beyond "anything other than crashing")

This simple function is int add(int a, int b) { return a + b ;}

How can I build a LLC with ubsan? I want to test in my local machine.

[dblaikie]

In D81631#2102343, @linzj wrote:

In D81631#2101784, @dblaikie wrote:

Could you include a comment in the test case explaining how this IR differs from all the IR clang usually generates? (in general what's interesting about this IR)

Also - remove the "valgrind" usage from that llc invocation (test machines might not have valgrind, etc - does UBSan/ASan/MSan catch this? There are buildbots that'll run the tests with sanitizers like those enabled & it'd be good if the test could catch a regression at least on those buildbots) - and include some testing of the behavior of llc (ie: what behavior do we get instead of UB? Presumably there's some specific behavior we're expecting beyond "anything other than crashing")

This simple function is int add(int a, int b) { return a + b ;}

Oh - hmm. I don't seem to reproduce this myself with "clang++ ^.cc -g -c -O3" running under valgrind. Is there something else I should be doing to reproduce/validate this?

How can I build a LLC with ubsan? I want to test in my local machine.

I believe you set the CMake variable: LLVM_USE_SANITIZER=Undefined

[linzj]

Oh - hmm. I don't seem to reproduce this myself with "clang++ ^.cc -g -c -O3" running under valgrind. Is there something else I should be doing to reproduce/validate this?

It should add -target armv7-linux-android. Or you should simply run llc with my test case.

[linzj]

I can't reproduce the bug with ubsan! Even in the latest version.
But valgrind is able to reproduce it.

[dblaikie]

In D81631#2102518, @linzj wrote:

I can't reproduce the bug with ubsan! Even in the latest version.

Memory sanitizer might be the better tool for this particular issue - if it's a read of an uninitialized value.

But valgrind is able to reproduce it.

Hmm - I'm not having much luck with that myself:

$ cat test.ll
target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "armv7-unknown-linux-android"

; Function Attrs: norecurse nounwind readnone
define hidden i32 @add(i32 %0, i32 %1) local_unnamed_addr #0 !dbg !9 {
  call void @llvm.dbg.value(metadata i32 %0, metadata !14, metadata !DIExpression()), !dbg !16
  call void @llvm.dbg.value(metadata i32 %1, metadata !15, metadata !DIExpression()), !dbg !16
  %3 = add nsw i32 %1, %0, !dbg !17
  ret i32 %3, !dbg !18
}

; Function Attrs: nounwind readnone speculatable willreturn
declare void @llvm.dbg.value(metadata, metadata, metadata) #1

attributes #0 = { norecurse nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+armv7-a,+d32,+dsp,+fp64,+neon,+vfp2,+vfp2sp,+vfp3,+vfp3d16,+vfp3d16sp,+vfp3sp,-crypto,-fp-armv8,-fp-armv8d16,-fp-armv8d16sp,-fp-armv8sp,-fp16,-fp16fml,-fullfp16,-thumb-mode,-vfp4,-vfp4d16,-vfp4d16sp,-vfp4sp" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { nounwind readnone speculatable willreturn }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5, !6, !7}
!llvm.ident = !{!8}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 (https://github.com/llvm/llvm-project.git 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "1.c", directory: "/tmp")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{i32 1, !"min_enum_size", i32 4}
!7 = !{i32 7, !"PIC Level", i32 2}
!8 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project.git 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)"}
!9 = distinct !DISubprogram(name: "add", scope: !1, file: !1, line: 1, type: !10, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
!10 = !DISubroutineType(types: !11)
!11 = !{!12, !12, !12}
!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!13 = !{!14, !15}
!14 = !DILocalVariable(name: "a", arg: 1, scope: !9, file: !1, line: 1, type: !12)
!15 = !DILocalVariable(name: "b", arg: 2, scope: !9, file: !1, line: 1, type: !12)
!16 = !DILocation(line: 0, scope: !9)
!17 = !DILocation(line: 2, column: 12, scope: !9)
!18 = !DILocation(line: 2, column: 3, scope: !9)$ cat test.ll
target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "armv7-unknown-linux-android"

; Function Attrs: norecurse nounwind readnone
define hidden i32 @add(i32 %0, i32 %1) local_unnamed_addr #0 !dbg !9 {
  call void @llvm.dbg.value(metadata i32 %0, metadata !14, metadata !DIExpression()), !dbg !16
  call void @llvm.dbg.value(metadata i32 %1, metadata !15, metadata !DIExpression()), !dbg !16
  %3 = add nsw i32 %1, %0, !dbg !17
  ret i32 %3, !dbg !18
}

; Function Attrs: nounwind readnone speculatable willreturn
declare void @llvm.dbg.value(metadata, metadata, metadata) #1

attributes #0 = { norecurse nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+armv7-a,+d32,+dsp,+fp64,+neon,+vfp2,+vfp2sp,+vfp3,+vfp3d16,+vfp3d16sp,+vfp3sp,-crypto,-fp-armv8,-fp-armv8d16,-fp-armv8d16sp,-fp-armv8sp,-fp16,-fp16fml,-fullfp16,-thumb-mode,-vfp4,-vfp4d16,-vfp4d16sp,-vfp4sp" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { nounwind readnone speculatable willreturn }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5, !6, !7}
!llvm.ident = !{!8}

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 (https://github.com/llvm/llvm-project.git 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "1.c", directory: "/tmp")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{i32 1, !"min_enum_size", i32 4}
!7 = !{i32 7, !"PIC Level", i32 2}
!8 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project.git 6dd738e2f0609f7d3313b574a1d471263d2d3ba1)"}
!9 = distinct !DISubprogram(name: "add", scope: !1, file: !1, line: 1, type: !10, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
!10 = !DISubroutineType(types: !11)
!11 = !{!12, !12, !12}
!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!13 = !{!14, !15}
!14 = !DILocalVariable(name: "a", arg: 1, scope: !9, file: !1, line: 1, type: !12)
!15 = !DILocalVariable(name: "b", arg: 2, scope: !9, file: !1, line: 1, type: !12)
!16 = !DILocation(line: 0, scope: !9)
!17 = !DILocation(line: 2, column: 12, scope: !9)
!18 = !DILocation(line: 2, column: 3, scope: !9)

Any ideas?

[linzj]

I think I have used to wrong sanitizer. Should be "Memory", not "Undefined".

[linzj]

Then the memory sanitizer finds another bug when compiling:

2397==WARNING: MemorySanitizer: use-of-uninitialized-value

#0 0xb210e7 in llvm::StringRef::find(char, unsigned long) const /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../include/llvm/ADT/StringRef.h:322:29
#1 0xb210e7 in llvm::StringRef::split(llvm::SmallVectorImpl<llvm::StringRef>&, char, int, bool) const /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../lib/Support/StringRef.cpp:344:20
#2 0xb2b6d4 in llvm::Triple::Triple(llvm::Twine const&) /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../lib/Support/Triple.cpp:739:19
#3 0xb63b3c in llvm::sys::getProcessTriple[abi:cxx11]() /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../lib/Support/Host.cpp:1615:10
#4 0xadecab in (anonymous namespace)::CommandLineParser::ParseCommandLineOptions(int, char const* const*, llvm::StringRef, llvm::raw_ostream*, bool) /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../lib/Support/CommandLine.cpp:1347:17
#5 0xadecab in llvm::cl::ParseCommandLineOptions(int, char const* const*, llvm::StringRef, llvm::raw_ostream*, char const*, bool) /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../lib/Support/CommandLine.cpp:1322:24
#6 0xa90283 in main /home/linzj/ssdsrc/llvm-project2/llvm/building-release/../utils/TableGen/TableGen.cpp:265:3
#7 0x7fc23a9d4b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
#8 0x426c69 in _start (/media/linzj/ff6fd1aa-94f2-4f61-9aba-67c6c99a0a59/home/linzj/src/llvm-project2/llvm/building-release/bin/llvm-tblgen+0x426c69)

[linzj]

The bug from libstdc++, switching to libc++.

[linzj]

use libc++ but the same error emitted!

[linzj]

I think memory sanitizer is buggy...

[dblaikie]

In D81631#2102864, @linzj wrote:

I think memory sanitizer is buggy...

Oh, sorry - I forgot to mention (& Had forgotten it entirely) that Memory Sanitizer requires a sanitized standard library - so it's a bit more involved/harder to get it working :/ I don't know all the details of how to get that setup and working.

Any ideas why my attempt to reproduce the issue with valgrind might not have worked as it has for you?

[linzj]

Any ideas why my attempt to reproduce the issue with valgrind might not have worked as it has for you?

The simplest way to reproduce is my test case. Use the same command in the first line.

[dblaikie]

Ah, I see - only reproduced in an optimized build. Sorry for the delays/noise.

I'd perhaps feel /marginally/ better about fixing this issue this way:

diff --git llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index 296c380ae55..8be0c19bbc4 100644
--- llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -333,13 +333,13 @@ DIE *DwarfCompileUnit::getOrCreateCommonBlock(
 void DwarfCompileUnit::addRange(RangeSpan Range) {
   DD->insertSectionLabel(Range.Begin);
 
-  bool SameAsPrevCU = this == DD->getPrevCU();
+  const DwarfCompileUnit *PrevCU = DD->getPrevCU();
   DD->setPrevCU(this);
   // If we have no current ranges just add the range and return, otherwise,
   // check the current section and CU against the previous section and CU we
   // emitted into and the subprogram was contained within. If these are the
   // same then extend our current range, otherwise add this as a new range.
-  if (CURanges.empty() || !SameAsPrevCU ||
+  if (CURanges.empty() || this != PrevCU ||
       (&CURanges.back().End->getSection() !=
        &Range.End->getSection())) {
     CURanges.push_back(Range);

At least I /think/ that should work & probably better reflect when this value should be used.

[linzj]

• bool SameAsPrevCU = this == DD->getPrevCU();

+ const DwarfCompileUnit *PrevCU = DD->getPrevCU();

Err, once you invoke getPrevCU, then a load-to-undefined instruction will be emitted. So valgrind will still report this problem. valgrind doesn't like undefined value's load.

[dblaikie]

In D81631#2141400, @linzj wrote:

• bool SameAsPrevCU = this == DD->getPrevCU();

+ const DwarfCompileUnit *PrevCU = DD->getPrevCU();

Err, once you invoke getPrevCU, then a load-to-undefined instruction will be emitted. So valgrind will still report this problem. valgrind doesn't like undefined value's load.

Right you are! Don't mind me then :) thanks for walking me through it all!

[linzj]

I think I should not attach this test case then.
You should not accept this patch. Instead you should accept the previous one.
Valgrind won't work in the Windows environment.

[dblaikie]

In D81631#2146742, @linzj wrote:

I think I should not attach this test case then.
You should not accept this patch. Instead you should accept the previous one.
Valgrind won't work in the Windows environment.

Yep - sure, the test case should not be included. (I don't think I can actually approve a specific prior revision of the patch in Phab) - are you able to commit the initialization or would you like me to?

[linzj]

Yep - sure, the test case should not be included. (I don't think I can actually approve a specific prior revision of the patch in Phab) - are you able to commit the initialization or would you like me to?

Yes. Thanks. I am not a commiter.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

 //===- llvm/CodeGen/DwarfDebug.h - Dwarf Debug Framework --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
class DwarfDebug : public DebugHandlerBase {
   SetVector<const DISubprogram *, SmallVector<const DISubprogram *, 16>,
             SmallPtrSet<const DISubprogram *, 16>>
       ProcessedSPNodes;
 
   /// If nonnull, stores the current machine function we're processing.
   const MachineFunction *CurFn = nullptr;
 
   /// If nonnull, stores the CU in which the previous subprogram was contained.
-  const DwarfCompileUnit *PrevCU;
+  const DwarfCompileUnit *PrevCU = nullptr;
 
   /// As an optimization, there is no need to emit an entry in the directory
   /// table for the same directory as DW_AT_comp_dir.
   StringRef CompilationDir;
 
   /// Holder for the file specific debug information.
   DwarfFile InfoHolder;