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

[Adrian Prantl via llvm-dev]

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