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

[David Blaikie]

Adrian - this disabled discriminators for DWARF < 4, but doesn't
disable them for OSX specifically. If OSX is using the default of
"current DWARF", it'll need a special case here, right? ("DWARF < 4 ||
darwin")

This change as-is will work for Chromium that uses -gdwarf-2 but might
not fix the wider darwin issue.

On Thu, Apr 17, 2014 at 1:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> ================
> 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
>
>