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

[David Blaikie]

On Thu, Apr 17, 2014 at 2:49 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> 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.

Fair enough, though at least this patch will bring darwin into a good
state - I just wanted to make sure we weren't just fixing this for
Chromium and leaving Darwin broken.

A flag can be added later, if you like.

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