[PATCH] D28005: Add a unit field to DIGlobalVariable, mirroring the design of DISubprogram.

[Adrian Prantl via llvm-commits]

> On Jan 4, 2017, at 9:56 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Jan 3, 2017 at 4:18 PM Adrian Prantl via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> aprantl added inline comments.
> 
> 
> ================
> Comment at: docs/SourceLevelDebugging.rst:452
> +   the variable's DICompileUnit.  If the global symbol the variable is attached
> +   to is linkonce_odr, and the compile unit the global variable gets emitted
> +   into doesn't matter, the DIGlobalVariable may instead be unique (i.e., not
> ----------------
> dblaikie wrote:
> > Probably could omit the reference to linkonce_odr and let the rest of the definition stand.
> >
> > That way if anything were to duplicate other globals (eg: thinlto might import globals, etc) they could/would use this functionality as well, potentially.
> `If the compile unit the global variable gets emitted into doesn't matter (such as a linkonce_odr symbol), ...`
> 
> 
> ================
> Comment at: docs/SourceLevelDebugging.rst:456-458
> +2. *Constant definitions* are reachable via the DICompileUnit's list of globals
> +   which points to a DIGlobalVariableExpression with a DIGlobalVariable without
> +   a unit field and a DIExpression describing a constant value.
> ----------------
> dblaikie wrote:
> > Is this true? Do we go and change the DIGlobalVariable if we collapse an llvm::GlobalValue into a constant? So that it doesn't have a unit field anymore? I assume we don't do that, so some constants might point to DIGlobalVariables with a non-null unit field.
> Good catch, I should implement this.
> 
> 
> ================
> Comment at: docs/SourceLevelDebugging.rst:460-461
> +
> +3. *Declarations* are DIGlobalVariables without a unit field that are only
> +   reachable via a DIImportedEntity and have isDefinition set to false.
> +
> ----------------
> dblaikie wrote:
> > Is this correct? If all DIGlobalVariables referenced by DIImportedEntities can have no unit field (& thus be emitted into any unit that references them) - should any DIGlobalVariables have unit fields?
> We added the unit field to DIGlobalVariables definitions (hanging off a global's !dbg attachment) so we can efficiently determine where to emit it. It's not strictly necessary, but it mirrors the DISubprogram design.
> 
> A pure declaration doesn't need one (and having one would prevent uniquing the DIImportedEntity) because it will be emitted as a locationless declaration in the CU it is being imported into (which should always be a correct one).
> 
> Is it? Could we end up with cross-module importing producing problems here?

This is how it works today (even before this patch). I tried compiling test/Bitcode/DIGlobalVariable-unit.ll (from the patch) and it inserts the pure forward decl of "h" into the CU the DIImportedEntity is in. I must admit I haven't thought through what this means in the ThinLTO case so far. My expectation is that in the worst case we will end up with a duplicate forward declaration in every CU that imports the variable. This is not very efficient, but it should work. However, if the forward declaration is not distinct, all identical  DIGlobalVariable nodes should be uniqued during importing and we'll only have one declaration. We cannot merge any forward declaration with a definition, though. Is this what you meant?
> 
> Local type and local inline function (guess that function probably doesn't get linkonce_odr, since it's local so it can't be duplicate with anything else) with a using declaration of that type in that function - then we import that function into another CU in ThinLTO - emit the function in that other CU, and end up emitting the type there too?
> 
I guess I would need to see an example to know for sure (and to make sure we're talking about the same thing).

-- adrian.

> Guess we don't have that situation today because the subprogram keeps the CU - and even if we remove the CU from inline functions we wouldn't remove it in this case because the function is local in my example.
> 
> Hrm. Still strikes me as a bit subtle/brittle, but maybe it's not.
> 
> - Dave
>  
> 
> 
> https://reviews.llvm.org/D28005 <https://reviews.llvm.org/D28005>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170104/ce7fdbe6/attachment.html>