[PATCH] D143229: [FunctionImporter] Don't upgrade debug info if llvm.idents match

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D143229#4103432 <https://reviews.llvm.org/D143229#4103432>, @arsenm wrote:

> In D143229#4103431 <https://reviews.llvm.org/D143229#4103431>, @arsenm wrote:
>
>> I don't think you can / should trust llvm.ident. It shouldn't be given a semantic meaning
>
> Plus this isn't even a single field, it's a list of many versions (unfortunately it's not deduplicated either)

Oh I misread the verifier code, yes you're right.

In D143229#4103619 <https://reviews.llvm.org/D143229#4103619>, @steven_wu wrote:

> In D143229#4103367 <https://reviews.llvm.org/D143229#4103367>, @aeubanks wrote:
>
>> In D143229#4103364 <https://reviews.llvm.org/D143229#4103364>, @steven_wu wrote:
>>
>>> In D143229#4103332 <https://reviews.llvm.org/D143229#4103332>, @aeubanks wrote:
>>>
>>>> check llvm.ident instead
>>>
>>> If my source module is bitcode upgraded, it won't get a new `llvm.ident` right? Then the upgrade pass will be wrongly skipped. I think we need a LLVM code generator version somewhere in the IR
>>
>> it the bitcode was upgraded, then during the upgrade it should already have had `UpgradeDebugInfo` called on it (with the expected LLVM revision) right?
>
> I am talking about A.bc imports function from B.bc. Both of bitcode files are out of date, so A.bc gets upgraded when loaded, then A tries to import B, they have the same ident so the upgrade from B is skipped.

I see, yes you're right.

In D143229#4107077 <https://reviews.llvm.org/D143229#4107077>, @dblaikie wrote:

> In the sense that if I write some handcrafted IR, LLVM should verify it before moving forward because other things interacting with the IR will assume it's valid & may crash if it isn't. So there's a trust boundary, generally - like in memory IR usually isn't verified, it's assumed the producer produced something valid by construction & it's a bug on the producer side if it didn't. But generally once we're reading things off disk we assume it might have come from anywhere & to be somewhat stable/not so crashy, we usually verify it on input (at least that was/is my rough understanding). So if you happen to be willing to sacrifice that extra stability guarantee because because you can guarantee that outside the system outside the program will ensure that only valid IR is passed in - then maybe having a flag to communicate that guarantee would be appropriate.

At this point I agree that a flag would be what I'd try for now.

In D143229#4107125 <https://reviews.llvm.org/D143229#4107125>, @mehdi_amini wrote:

> I guess it depends what the contract is for `UpgradeDebugInfo`, because from the name it didn't seem to me like a "verifier"?

`UpgradeDebugInfo` runs the verifier, and the verifier has a mode that doesn't fail if the debug info is invalid, but rather reports it separately. We could separate out the debug info portion of the verifier into a separate `DIVerifier` and use that instead. But we're still doing unnecessary work verifying debug info that we can assume to be valid in our build environments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143229/new/

https://reviews.llvm.org/D143229