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

[David Blaikie]

================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:163
@@ +162,3 @@
+  // do nothing (discriminator support is a DWARF 4 feature).
+  if (!hasDebugInfo(F) ||
+      dwarf::getDwarfVersionFromModule(F.getParent()) < 4 ||
----------------
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.

================
Comment at: test/Transforms/AddDiscriminators/no-discriminators.ll:1
@@ +1,2 @@
+; RUN: opt < %s -add-discriminators -S | FileCheck %s
+
----------------
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).

================
Comment at: test/Transforms/AddDiscriminators/no-discriminators.ll:70
@@ +69,3 @@
+!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}
----------------
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).

================
Comment at: include/llvm/Support/Dwarf.h:957
@@ +956,3 @@
+/// Return Dwarf Version by checking module flags.
+inline unsigned getDwarfVersionFromModule(const Module *M) {
+  Value *Val = M->getModuleFlag("Dwarf Version");
----------------
This might be worth being out of line, maybe. Not a big deal.


http://reviews.llvm.org/D3413