On Tue, Jul 2, 2013 at 5:45 PM, Manman Ren <[email protected]> wrote: > > On Jul 2, 2013, at 5:40 PM, Eric Christopher wrote: > >> I see you went ahead and committed these (the mailing list is down, >> but I saw the updates via fabricator). > > I did ping once, and didn't get any reply. And it seems that nobody has > strong opinion on one over the other. > > If we want to change it, it is not that hard to do :) >
In the long run I think we'll end up changing it. Please do implement the functionality that I requested though. Thanks. -eric > Manman > >> >> 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 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. 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
