[llvm-dev] Why does a DISubprogram need to be distinct?

[David Blaikie via llvm-dev]

On Fri, Sep 25, 2020 at 5:20 PM Adrian Prantl <aprantl at apple.com> wrote:

>
>
> On Sep 25, 2020, at 4:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Fri, Sep 25, 2020 at 4:08 PM Adrian Prantl <aprantl at apple.com> wrote:
>
>
> First — thanks for fixing the test for me!
>
>
> I'm a bit curious about the test - any idea how it came to be, if it's
> invalid? Did we produce such bitcode in the past and don't anymore -
> what's the rule about backwards compatibility here, then? It seems
> like any time we regenerate a bitcode compat test that's questionable
> because it means we won't be compatible with that bitcode we said we
> should be compatible with?
>
>
> I think I mentioned in the commit message — we had a short period in clang
> with a bug
>
> >>> Clang right before https://reviews.llvm.org/D79967 would generate
> this kind of broken IR.
> and the swift-5.3 branch happened to be cut in that unlucky window. To
> trigger it you need to markup a function with NoDebug (the bug was
> introduced  maybe a month earlier).
>
 (snipped from other email):
>
I didn't answer the bitcode part: Adding the Verifier check *is* the
> bitcode compatibility. If an AssertDI fires, the debug info is stripped
> from the CU with a warning about invalid debug info. We don't need be
> implement a bitcode upgrade for a buggy version of clang, particularly not
> in this case, since I also cherry-picked the bugfix to that same branch.
>

Ah, OK, all makes sense. So invalid DWARF isn't invalid IR/rejected, it's
just dropped (knew that on some level, but hadn't put it all together
properly - appreciate you covering it), so some quirky IR in the past we
can accept is buggy rather than just old and drop rather than handle.


>
>
> We have two kinds of DISubprograms:
> - uniqued DISubprograms are part of the type system and describe function
> *types*. They don't have a unit: field, because they want to be uniqued
> across compile units.
> - distinct DISubprograms describe one specific function definition. They
> belong to a compile unit and thus have a unit: field.
>
> You are only supposed to attach distinct DISubprograms to function
> definitions. Several places in CodeGen will unconditionally dereference the
> unit: field and crash if it isn't there.
>
>
> But do they have to be distinct? They have different fields (as you
> say, declaration DISubprograms don't have a "unit:" field, definition
> DISubprograms do have a unit: field that refers to a (itself distinct)
> DICompileUnit)
>
>
> IIRC, DIBuilder lets you either create a distinct one with a unit or a
> uniqued one without.
>

I'm with you on the implementation, but trying to understand the
justification.


> I don't know if the backend would survive the same uniqued DISubprogram
> node being attached to multiple llvm::Functions or if that would invalidate
> some assumptions... In any case distinct seems to be a reasonable proxy for
> what we mean.
>

Right, so maybe the clearer question would be: What would go wrong if
DISubprogram wasn't distinct.

I don't think distinct stops a DISubprogram from being referred to from
multiple llvm::Functions at the same time - it just stops the IR linker
from deduplicating the DISubprograms when IR linking, I think? But since
the DISubprograms refer to the DICompileUnit which is distinct, so the
DISubprograms wouldn't be the same (they'd be referring to different
DICompileUnits) so they wouldn't be deduplicated across IR modules when IR
linking.

Perhaps marking them as distinct saves some link time by not requiring the
IR Linker from /trying/ to merge them? So the generalization of that
observation would be "any IR that refers to a distinct node can and should
(but doesn't have to be) distinct itself to simplify/speed up IR linking,
because it can't ever be deduplicated anyway"?


>
> -- adrian
>
>
> We actually had a weaker form of the check I added to Verifier already in
> place (
> https://github.com/llvm/llvm-project/blob/51cad041e0cb26597c7ccc0fbfaa349b8fffbcda/llvm/lib/IR/Verifier.cpp#L1186),
> but it didn't cover the specific testcase I added in
> https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4.
>
> -- adrian
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200925/e7a5d4c1/attachment.html>