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

[Adrian Prantl]

On Apr 17, 2014, at 1:12 PM, David Blaikie <dblaikie at gmail.com> wrote:

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

No I think this is fine. DWARF v2 is the default on Darwin and I won’t change that until all tools support it. But I’d rather have a way to enable the feature for those who want to live on the bleeding edge than disabling it completely.

thanks,
-- adrian


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