r184276 - Debug Info: support for gdwarf-2 gdwarf-3 gdwarf-4

[Eric Christopher]

On Wed, Jul 3, 2013 at 5:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Jul 2, 2013 5:40 PM, "Eric Christopher" <echristo at gmail.com> wrote:
>>
>> I see you went ahead and committed these (the mailing list is down,
>> but I saw the updates via phabricator).
>>
>> The discussion may have faltered, but given that there was
>> disagreement on the original patch I would think that it's your
>> responsibility to ping the thread before committing.
>>
>> I've spoken with Dan again and we've chatted about the module flag
>> versus in the compile unit and the rationale for the direction you've
>> chosen. Overall while I think it should be the other way
>
> Could we try to capture some/more of the rationale in both directions
> actually in email rather than (or at least in addition to) offline
> conversations so we can refer to it later. As it stands it seems were still
> lacking any of that conversation in an archived form.

*nod*

Basically to summarize it's pretty much what you said above: when
we're linking, we a) have full control over what we emit, and b) it's
a specialized enough use case that following what most debug info does
isn't as compelling an argument. I do think being able to limit
features on a per-cu basis is worth it, but at this point it's
probably a fine point rather than absolute correctness.

>
>> it's not
>> overly damaging to llvm for it to be the direction that you've already
>> written the code and can always be changed later if we find it too
>> limiting.
>>
>> That said, I do believe that the warning should go away and that you
>> should fix the code (I guess during module linking since you've gone
>> this direction) so that it sorts out the values and re-sets the module
>> flag to the minimum of all modules.
>
> What's the reasoning for this choice?it would seem to me that the maximum
> would make a fair bit of sense, though I admit it does seem somewhat
> arbitrary either way.
>

The idea is that whomever decided to limit their debug info to, say,
dwarf-2 should win overall because they theoretically know better
(since they specified an option) what the capabilities of the system
are. i.e. It's the conservative choice to make.

-eric

>> This will mean that a number of
>> more advanced features may get turned off, but it's the conservative
>> answer.
>>
>> -eric
>>
>>
>>
>> On Thu, Jun 20, 2013 at 4:48 PM, Manman Ren <mren at apple.com> wrote:
>> >
>> > On Jun 19, 2013, at 4:28 PM, Eric Christopher wrote:
>> >
>> >>>> I don't think a module flag is the way we want to go about this, I
>> >>>> think it should be an attribute on the compile unit created.
>> >>>
>> >>> Hmm, I could've sworn we (Eric, Dan, and myself) in person a couple of
>> >>> weeks ago & decided that a module flag was the way to go - but I'm not
>> >>> 100% sure that's how the conversation concluded & I'm also not sure I
>> >>> can reconstruct all the arguments.
>> >>>
>> >>> I think it went something like: we'll need the module-level
>> >>> functionality anyway, and it'll be able to report
>> >>> errors/incompatibilities in a uniform way that users will be used to,
>> >>> whereas if we did this down in debug info we wouldn't be able to use
>> >>> such error reporting if there are "incompatible" dwarf version
>> >>> requests. (I suppose there might not be - in which case it'd be "min"
>> >>> or "max" essentially and even then, presumably, having the common
>> >>> module functionality able to handle that for us seems reasonable/nice)
>> >>>
>> >>
>> >> Right, but that pretty much contradicts how linkers deal with them
>> >> today - it's perfectly compatible to link them without warning.
>> >>
>> >>>> Few constraints there:
>> >>>>
>> >>>> a) this means that we just check the compile unit as we create up the
>> >>>> DIE and we don't have to check the module.
>> >>>
>> >>> I'm not sure that's a huge change - and we'd still have to have some
>> >>> code resolve the difference between different modules & presumably
>> >>> rewrite the compile unit of each one to bring it up to whatever the
>> >>> One True dwarf version is that we'll be using for this build, no? Or
>> >>> are you expecting to generate some DWARF-N for some compile units in
>> >>> the same compilation and DWARF-M for others?
>> >>
>> >> Only at LTO time and only up to a minimum of 3. Otherwise, I'm
>> >> perfectly fine with generating differing values of dwarf for each
>> >> compile unit.
>> >>
>> >>> I'm not sure how we settled on this issue - whether the module
>> >>> handling should just bump everything up to the "minimum compatible"
>> >>> version (floor of DWARF-4 or whatever, otherwise the max of all the
>> >>> specified versions from the different CUs).
>> >>>
>> >>>> c) at LTO time we'll likely want to constrain to a minimum dwarf
>> >>>> version that contains ref addr so that we can minimize, but this is
>> >>>> by
>> >>>> no means required,
>> >>>
>> >>> Fair enough, so maybe the 'floor' above isn't required.
>> >>>
>> >>
>> >> *nod* Basically the way we're doing LTO is going to require
>> >> inter-compile unit references. Other than that there's no real need to
>> >> do any munging of the value at all. Hence me figuring to just leave it
>> >> on each compile unit and keep the logic on what to emit in the compile
>> >> unit itself.
>> >>
>> >> It probably comes down to simple preference really there's nothing
>> >> compelling about either way unless we want to explicitly warn people
>> >> and that'd be creating a new warning for "little" reason and it might
>> >> be surprising for people that were previously getting no warnings with
>> >> normal compilation moving to LTO. That said, maybe they'll be
>> >> surprised when they get inter-compile unit references.
>> > Any conclusion here? I am okay with either choice, and about to modify
>> > the backend.
>> >
>> > Manman
>> >
>> >>
>> >> *shrug*
>> >>
>> >> -eric
>> >>
>> >>
>> >>>> d) it matches how linkers actually link debug information
>> >>>>
>> >>>> Also:
>> >>>>
>> >>>>> +  else if (Opts.getDebugInfo() != CodeGenOptions::NoDebugInfo)
>> >>>>> +    // Default Dwarf version is 3 if we are generating debug
>> >>>>> information.
>> >>>>> +    Opts.DwarfVersion = 3;
>> >>>>>
>> >>>>
>> >>>> We want 4 here I believe, or even a level for "latest" with all of
>> >>>> the
>> >>>> bells and whistles. 4 is a likely sufficient start though since we'd
>> >>>> have to define what "all the bells and whistles" means :) Or, as an
>> >>>> alternate question, "why 3?"
>> >>>>
>> >>>> Thanks!
>> >>>>
>> >>>> -eric
>> >>>> _______________________________________________
>> >>>> cfe-commits mailing list
>> >>>> cfe-commits at cs.uiuc.edu
>> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >