[PATCH] D99334: [TransformUtils] Don't generate invalid llvm.dbg.cu

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D99334#2702363 <https://reviews.llvm.org/D99334#2702363>, @ldrumm wrote:

> In D99334#2702227 <https://reviews.llvm.org/D99334#2702227>, @dblaikie wrote:
>
>> In D99334#2701760 <https://reviews.llvm.org/D99334#2701760>, @ldrumm wrote:
>>
>>>> Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?
>>>
>>> Because 3 lines later we have `  auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");`
>>> In the case that the source module has no `llvm.dbu.cu` the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.
>>
>> I don't understand why it would be either illegal or a pain to deal with. (& I'm not sure it's illegal either - I modified an example IR file to have an empty llvm.dbg.cu list and it seemed to compile file (compared to other violations of IR debug info verification, which did result in an error/warning like this:
>>
>>   inlinable function call in a function with debug info must have a !dbg location
>>     call void @_Z2f1v()
>>   warning: ignoring invalid debug info in test.ll
>>   ```)
>
>
>
> 1. The verifier says it's illegal.

Where does it say that? it doesn't look to me like this would fail with a zero-length llvm.dbg.cu: https://github.com/microsoft/llvm/blob/eddbfad05ce3a85732d8bff9f85ef63afc97ad7e/lib/IR/Verifier.cpp#L758 & as mentioned in my last message, my testing seemed to show that a zero-length llvm.dbg.cu didn't trigger a failure in the IR verifier, unlike some IR I knew to be invalid (missing dbg location on a call in a dbg-having function)

> When I wrote this patch the regression test included fails without this change

The unit test? It specifically tests for llvm.dbg.cu being null at the end, and it seems that's what fails without the code change - but I'm suggesting that test is incorrect, and that it's OK for llvm.dbg.cu to be non-null (but empty) here.

> 2. getting an `llvm.dbg.cu`, and then having to check it's got the right number of operands so you don't crash every time is a pain.

I don't understand what code you're suggesting here - it looks to me like all the LLVM code that visits llvm.dbg.cu iterates over it not caring about the total number of elements - if there are zero, the loop iterates zero times (see the verifier code linked above), if it's 10, the loop iterates 10 times - either is totally fine.

> If you're still unhappy with this `if` statement can you suggest how I solve this bug in a more suitable way?

I'm suggesting this patch is not necessary, and the behavior of the existing code doesn't seem to me to be buggy.


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

https://reviews.llvm.org/D99334