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

Tue Apr 20 10:56:45 PDT 2021

ldrumm added a comment.

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. When I wrote this patch the regression test included fails without this change
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. It's partly about ergonomics - and partly about making it easy for users to do the right thing.

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

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

https://reviews.llvm.org/D99334