On Wed, Jul 3, 2013 at 5:24 PM, David Blaikie <[email protected]> wrote: > > On Jul 2, 2013 5:40 PM, "Eric Christopher" <[email protected]> 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 <[email protected]> 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 >> >>>> [email protected] >> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
