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

[David Blaikie via llvm-commits]

On Tue, Jan 3, 2017 at 4:18 PM Adrian Prantl via Phabricator <
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?

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?

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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170104/3fa22698/attachment.html>