llvm dev - Sep 2020 - Why does a DISubprogram need to be distinct?

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?
> 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)
> 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

> 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
<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).
> 
>> 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 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.

-- 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/1e3e1c58/attachment.html>

> On 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
<mailto:dblaikie at gmail.com>> wrote:
>> 
>> On Fri, Sep 25, 2020 at 4:08 PM Adrian Prantl <aprantl at apple.com
<mailto: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 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.

-- adrian
> 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
<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).
> 
>> 
>>> 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 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.
> 
> -- 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
<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
<https://reviews.llvm.org/rGe17f52d623cc146b7d9bf5a2e02965043508b4c4>.
>>> 
>>> -- adrian
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200925/65336414/attachment-0001.html>

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>