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

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

lib/Transforms/Utils/AddDiscriminators.cpp

163

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

1 ↗

(On Diff #8611)

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).

70 ↗

(On Diff #8611)

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

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

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

1 ↗

(On Diff #8611)

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 ↗

(On Diff #8613)

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

607 ↗

(On Diff #8613)

Any reason not to return the strongly typed enum?

[dnovillo]

• Use proper doxygen markup for return value.

[dblaikie]

LGTM

include/llvm/Support/Dwarf.h

 #ifndef LLVM_SUPPORT_DWARF_H
 #define LLVM_SUPPORT_DWARF_H
 
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataTypes.h"
 
 namespace llvm {
 
+class Module;
+
 //===----------------------------------------------------------------------===//
 // Debug info constants.
 
 enum : uint32_t {
   LLVMDebugVersion = (12 << 16),    // Current version of debug information.
   LLVMDebugVersion11 = (11 << 16),  // Constant for version 11.
   LLVMDebugVersion10 = (10 << 16),  // Constant for version 10.
   LLVMDebugVersion9 = (9 << 16),    // Constant for version 9.
private:
   enum {
     KIND_OFFSET = 4,
     KIND_MASK = 7 << KIND_OFFSET,
     LINKAGE_OFFSET = 7,
     LINKAGE_MASK = 1 << LINKAGE_OFFSET
   };
 };
 
+/// Return Dwarf Version by checking module flags.
+unsigned getDwarfVersionFromModule(const Module *M);
+
 } // End of namespace dwarf
 
 } // End of namespace llvm
 
 #endif

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());
+                     : dwarf::getDwarfVersionFromModule(MMI->getModule());
 
   {
     NamedRegionTimer T(DbgTimerName, DWARFGroupName, TimePassesIsEnabled);
     beginModule();
   }
 }
 
 // Switch to the specified MCSection and emit an assembler

lib/Support/Dwarf.cpp

 //===-- llvm/Support/Dwarf.cpp - Dwarf Framework ----------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains support for generic dwarf information.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Constants.h"
 
 using namespace llvm;
 using namespace dwarf;
 
 /// TagString - Return the string for the specified tag.
 ///
 const char *llvm::dwarf::TagString(unsigned Tag) {
   switch (Tag) {
const char *llvm::dwarf::GDBIndexEntryLinkageString(GDBIndexEntryLinkage Linkage) {
   switch (Linkage) {
   case GIEL_EXTERNAL:
     return "EXTERNAL";
   case GIEL_STATIC:
     return "STATIC";
   }
   llvm_unreachable("Unknown GDBIndexEntryLinkage value");
 }
+
+unsigned getDwarfVersionFromModule(const Module *M) {
+  Value *Val = M->getModuleFlag("Dwarf Version");
+  if (!Val)
+    return llvm::dwarf::DWARF_VERSION;
+  return cast<ConstantInt>(Val)->getZExtValue();
+}

lib/Transforms/Utils/AddDiscriminators.cpp

 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Dwarf.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
 namespace {
   struct AddDiscriminators : public FunctionPass {
     static char ID; // Pass identification, replacement for typeid
     AddDiscriminators() : FunctionPass(ID) {

 /// 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;
+  // Finally, if this module is built with dwarf versions earlier than 4,
+  // do nothing (discriminator support is a DWARF 4 feature).
+  if (!hasDebugInfo(F) ||
+      dwarf::getDwarfVersionFromModule(F.getParent()) < 4 ||
+      NoDiscriminators)
+    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.