[llvm] Allow 128-bit discriminants in DWARF variants (PR #125578)
Tom Tromey via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 4 06:32:18 PST 2025
tromey wrote:
> I think that's probably still the right path - what unrelated test changes did you discover down this path?
If `addConstantValue` calls the new `addInt`, then different forms can be emitted in some tests. This is because `addUInt` and `addSInt` use `DIEInteger::BestForm`, while the `uint64_t` overload of `addConstantValue` has this:
```
// FIXME: This is a bit conservative/simple - it emits negative values always
// sign extended to 64 bits rather than minimizing the number of bytes.
addUInt(Die, dwarf::DW_AT_const_value,
Unsigned ? dwarf::DW_FORM_udata : dwarf::DW_FORM_sdata, Val);
```
I tend to think this FIXME can be removed, since DWARF recommends what LLVM is doing here. See 7.5.5 Classes and Forms subheading "constant":
> If one of the DW_FORM_data<n>forms is used to represent a signed or unsigned
> integer, it can be hard for a consumer to discover the context necessary to
> determine which interpretation is intended. Producers are therefore strongly
> encouraged to use DW_FORM_sdata or DW_FORM_udata for signed and
> unsigned integers respectively, rather than DW_FORM_data<n>.
> Potentially doing this in two steps, even - one, an API change to `DwarfUnit::addConstantValue` take the `DW_AT_` to use as a parameter. That change shouldn't cause any change to LLVM's behavior.
>
> Then, after that, we could reuse the function from this new location.
My first take on the patch did this, but in the end I chose to add a new method because `addConstantValue` by its name seemed like it is intended to be specific to `DW_AT_const_value`.
There's also `addConstantFPValue`, where the aforementioned DWARF section seems to imply that `DW_FORM_udata` should not be used -- rather `DW_FORM_data8` -- see the second paragraph. I feel this probably doesn't matter to readers in practice.
By "two steps" do you mean opening two separate pull requests? I'm new to LLVM and my impression is that patch series aren't really done, but I wanted to confirm.
Anyway I'm happy to proceed whatever way you like, just let me know, thanks.
https://github.com/llvm/llvm-project/pull/125578
More information about the llvm-commits
mailing list