Fix bug 19437 - Only add discriminators for DWARF 4 and above.

[dnovillo]

Summary

This prevents the discriminator generation pass from triggering if
the DWARF version being used in the module is prior to 4.

Note: The test case for this is in Clang, as this is only triggered

during Clang's code generation.

[dblaikie]

Is there any reason the AddDescriminator pass isn't part of llc/opt/etc so it can be tested in LLVM?

[dnovillo]

Add forgotten test.

I forgot to git add this test before sending the original patch.

[dnovillo]

Move getDwarfVersionFromModule into Dwarf.h.

[echristo]

Hrm. Don't know that we can make Support depend on IR any more than we already do. Chandler? Opinions?

[dblaikie]

include/llvm/Support/Dwarf.h

957 ↗

(On Diff #8611)

This might be worth being out of line, maybe. Not a big deal.

lib/Transforms/Utils/AddDiscriminators.cpp

161

I'd probably order this in terms of least expensive to most, which I assume is:

NoDiscriminators ||
version < 4 ||
hasDebugInfo(F)

(but I haven't looked at the implementation of hasDebugInfo to see if it's the most costly)

And rephrase the comment to match the order?

Not necessary, just a thought, struck me as odd to have the comment say "Finally, " but the version check not to be the last thing.

test/Transforms/AddDiscriminators/no-discriminators.ll

2

You should be able to take an existing test case and just ad another RUN line with a specified dwarf version (there should be an opt flag to override the version... but it looks like that flag only works in DwarfDebug & would need to be moved/refactored in some way (possibly as a real option for opt/llc/llvm-mc/etc)) & demonstrate that the discriminators no longer appear. We have tests like this for other dwarf version-specific features already (but they're all in DWARF generation in DwarfDebug, rather than an opt pass like this).

71

Having the check lines way down here is a bit hard to spot - reckon it'd be better to put them up the top just after the source?

What's the modification you're trying to ensure did not occur? Testing for the exact metadata node match is a bit limiting to other changes to the debug info which we prefer not to do (because it makes it hard to change debug info without having to fix a bunch of unrelated tests).

[chandlerc]

include/llvm/Support/Dwarf.h

957 ↗

(On Diff #8611)

As Eric was indicating, this can't be in Support. I think this should be a method on Module personally. It needs to be somewhere in the IR library at least.

[dblaikie]

include/llvm/Support/Dwarf.h

957 ↗

(On Diff #8611)

Yeah, a method on Module sounds pretty reasonable to me. (it's a bit special case, I wouldn't mind a generic system for retrieving module flags with defaults, but not fussed)

[dnovillo]

• Move getDwarfVersionFromModule to Module::getDwarfVersion.
• Re-arrange the checks when deciding whether to add discriminators.

[dnovillo]

• Remove extraneous doxygen marker.

[dnovillo]

test/Transforms/AddDiscriminators/no-discriminators.ll

2

The pass can only rely on what's in the IR. It's not affected by flags, other than the specific one that tells it not to bother. Not sure I'm following you here.

[chandlerc]

Ok, nits from the peanut gallery, but feel free to ignore me and listen to the real reviewers. ;]

/me runs away

include/llvm/IR/Module.h

606

Go ahead and put a '\brief' on this.

607

Any reason not to return the strongly typed enum?

[dnovillo]

• Use proper doxygen markup for return value.

[dblaikie]

LGTM

include/llvm/IR/Module.h

/// @{
   
   /// This function causes all the subinstructions to "let go" of all references
   /// that they are maintaining.  This allows one to 'delete' a whole class at
   /// a time, even though there may be circular references... first all
   /// references are dropped, and all use counts go to zero.  Then everything
   /// is delete'd for real.  Note that no operations are valid on an object
   /// that has "dropped all references", except operator delete.
   void dropAllReferences();
+
+/// @}
+/// @name Utility functions for querying Debug information.
+/// @{
+
+  /// \returns the Dwarf Version by checking module flags.
+  unsigned getDwarfVersion() const;
+
 /// @}
 };
 
 /// An raw_ostream inserter for modules.
 inline raw_ostream &operator<<(raw_ostream &O, const Module &M) {
   M.print(O, nullptr);
   return O;
 }

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (unsigned i = 0, N = Elements.getNumElements(); i < N; ++i) {
         return (resolve(DT.getTypeDerivedFrom()));
     }
   }
   return Ty;
 }
 
 } // end llvm namespace
 
-/// Return Dwarf Version by checking module flags.
-static unsigned getDwarfVersionFromModule(const Module *M) {
-  Value *Val = M->getModuleFlag("Dwarf Version");
-  if (!Val)
-    return dwarf::DWARF_VERSION;
-  return cast<ConstantInt>(Val)->getZExtValue();
-}
-
 DwarfDebug::DwarfDebug(AsmPrinter *A, Module *M)
     : Asm(A), MMI(Asm->MMI), FirstCU(0), PrevLabel(NULL), GlobalRangeCount(0),
       InfoHolder(A, "info_string", DIEValueAllocator),
       UsedNonDefaultText(false),
       SkeletonHolder(A, "skel_string", DIEValueAllocator) {
 
   DwarfInfoSectionSym = DwarfAbbrevSectionSym = DwarfStrSectionSym = 0;
   DwarfDebugRangeSectionSym = DwarfDebugLocSectionSym = DwarfLineSectionSym = 0;
DwarfDebug::DwarfDebug(AsmPrinter *A, Module *M)
 
   if (DwarfPubSections == Default)
     HasDwarfPubSections = !IsDarwin;
   else
     HasDwarfPubSections = DwarfPubSections == Enable;
 
   DwarfVersion = DwarfVersionNumber
                      ? DwarfVersionNumber
-                     : getDwarfVersionFromModule(MMI->getModule());
+                     : MMI->getModule()->getDwarfVersion();
 
   {
     NamedRegionTimer T(DbgTimerName, DWARFGroupName, TimePassesIsEnabled);
     beginModule();
   }
 }
 
 // Switch to the specified MCSection and emit an assembler

lib/IR/Module.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/GVMaterializer.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/LeakDetector.h"
+#include "llvm/Support/Dwarf.h"
 #include <algorithm>
 #include <cstdarg>
 #include <cstdlib>
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 // Methods to implement the globals and functions lists.
 //
for(Module::iterator I = begin(), E = end(); I != E; ++I)
     I->dropAllReferences();
 
   for(Module::global_iterator I = global_begin(), E = global_end(); I != E; ++I)
     I->dropAllReferences();
 
   for(Module::alias_iterator I = alias_begin(), E = alias_end(); I != E; ++I)
     I->dropAllReferences();
 }
+
+unsigned Module::getDwarfVersion() const {
+  Value *Val = getModuleFlag("Dwarf Version");
+  if (!Val)
+    return dwarf::DWARF_VERSION;
+  return cast<ConstantInt>(Val)->getZExtValue();
+}

lib/Transforms/Utils/AddDiscriminators.cpp

 /// To decide where to create new discriminator values, this function
 /// traverses the CFG and examines instruction at basic block boundaries.
 /// If the last instruction I1 of a block B1 is at the same file and line
 /// location as instruction I2 of successor B2, then it creates a new
 /// lexical block for I2 and all the instruction in B2 that share the same
 /// file and line location as I2. This new lexical block will have a
 /// different discriminator number than I1.
 bool AddDiscriminators::runOnFunction(Function &F) {
-  // No need to do anything if there is no debug info for this function.
   // If the function has debug information, but the user has disabled
   // discriminators, do nothing.
-  if (!hasDebugInfo(F) || NoDiscriminators) return false;
+  // Simlarly, if the function has no debug info, do nothing.
+  // Finally, if this module is built with dwarf versions earlier than 4,
+  // do nothing (discriminator support is a DWARF 4 feature).
+  if (NoDiscriminators ||
+      !hasDebugInfo(F) ||
+      F.getParent()->getDwarfVersion() < 4)
+    return false;
 
   bool Changed = false;
   Module *M = F.getParent();
   LLVMContext &Ctx = M->getContext();
   DIBuilder Builder(*M);
 
   // Traverse all the blocks looking for instructions in different
   // blocks that are at the same file:line location.

test/Transforms/AddDiscriminators/no-discriminators.ll

+; RUN: opt < %s -add-discriminators -S | FileCheck %s
+
+; We should not generate discriminators for DWARF versions prior to 4.
+;
+; Original code:
+;
+; int foo(long i) {
+;   if (i < 5) return 2; else return 90;
+; }
+;
+; None of the !dbg nodes associated with the if() statement should be
+; altered. If they are, it means that the discriminators pass added a
+; new lexical scope.
+
+define i32 @foo(i64 %i) #0 {
+entry:
+  %retval = alloca i32, align 4
+  %i.addr = alloca i64, align 8
+  store i64 %i, i64* %i.addr, align 8
+  call void @llvm.dbg.declare(metadata !{i64* %i.addr}, metadata !13), !dbg !14
+  %0 = load i64* %i.addr, align 8, !dbg !15
+; CHECK:  %0 = load i64* %i.addr, align 8, !dbg !15
+  %cmp = icmp slt i64 %0, 5, !dbg !15
+; CHECK:  %cmp = icmp slt i64 %0, 5, !dbg !15
+  br i1 %cmp, label %if.then, label %if.else, !dbg !15
+; CHECK:  br i1 %cmp, label %if.then, label %if.else, !dbg !15
+
+if.then:                                          ; preds = %entry
+  store i32 2, i32* %retval, !dbg !15
+  br label %return, !dbg !15
+
+if.else:                                          ; preds = %entry
+  store i32 90, i32* %retval, !dbg !15
+  br label %return, !dbg !15
+
+return:                                           ; preds = %if.else, %if.then
+  %1 = load i32* %retval, !dbg !17
+  ret i32 %1, !dbg !17
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata) #1
+
+attributes #0 = { nounwind uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!10, !11}
+!llvm.ident = !{!12}
+
+!0 = metadata !{i32 786449, metadata !1, i32 12, metadata !"clang version 3.5.0 ", i1 false, metadata !"", i32 0, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2, metadata !"", i32 1} ; [ DW_TAG_compile_unit ] [./no-discriminators] [DW_LANG_C99]
+!1 = metadata !{metadata !"no-discriminators", metadata !"."}
+!2 = metadata !{}
+!3 = metadata !{metadata !4}
+!4 = metadata !{i32 786478, metadata !1, metadata !5, metadata !"foo", metadata !"foo", metadata !"", i32 1, metadata !6, i1 false, i1 true, i32 0, i32 0, null, i32 256, i1 false, i32 (i64)* @foo, null, null, metadata !2, i32 1} ; [ DW_TAG_subprogram ] [line 1] [def] [foo]
+!5 = metadata !{i32 786473, metadata !1}          ; [ DW_TAG_file_type ] [./no-discriminators]
+!6 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0, i64 0, i32 0, null, metadata !7, i32 0, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{metadata !8, metadata !9}
+!8 = metadata !{i32 786468, null, null, metadata !"int", i32 0, i64 32, i64 32, i64 0, i32 0, i32 5} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
+!9 = metadata !{i32 786468, null, null, metadata !"long int", i32 0, i64 64, i64 64, i64 0, i32 0, i32 5} ; [ DW_TAG_base_type ] [long int] [line 0, size 64, align 64, offset 0, enc DW_ATE_signed]
+!10 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
+; CHECK: !10 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
+!11 = metadata !{i32 1, metadata !"Debug Info Version", i32 1}
+!12 = metadata !{metadata !"clang version 3.5.0 "}
+!13 = metadata !{i32 786689, metadata !4, metadata !"i", metadata !5, i32 16777217, metadata !9, i32 0, i32 0} ; [ DW_TAG_arg_variable ] [i] [line 1]
+!14 = metadata !{i32 1, i32 0, metadata !4, null}
+!15 = metadata !{i32 2, i32 0, metadata !16, null}
+; CHECK: !15 = metadata !{i32 2, i32 0, metadata !16, null}
+!16 = metadata !{i32 786443, metadata !1, metadata !4, i32 2, i32 0, i32 0, i32 0} ; [ DW_TAG_lexical_block ] [./no-discriminators]
+; CHECK: !16 = metadata !{i32 786443, metadata !1, metadata !4, i32 2, i32 0, i32 0, i32 0} ; [ DW_TAG_lexical_block ] [./no-discriminators]
+!17 = metadata !{i32 3, i32 0, metadata !4, null}