[PATCH] D75527: [yaml2obj] - Add `ELFYAML::YAMLInt` to fix how we parse a relocation `Addend` key.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D75527#1909495 <https://reviews.llvm.org/D75527#1909495>, @grimar wrote:

> In D75527#1907129 <https://reviews.llvm.org/D75527#1907129>, @jhenderson wrote:
>
> > My inclination is that yaml2obj should support both signed and unsigned variants of both hex and decimal numbers, if feasible.
>
>
> I'd like to clarify what you mean.
>
> Are you ok to support `Addend: -4294967295`? (4294967295 == 0xFFFFFFFF) for 32 bit ELF
>  and `Addend: -18446744073709551615` (18446744073709551615 == 0xFFFFFFFFFFFFFFFF) for 64-bit one?
>  (it is the same as `Addend: 1`)
>
> Looks like that because we are not going to restrict
>  `Addend: -0xFFFFFFFF` or `Addend: -0xFFFFFFFFFFFFFFFF` it seems.
>
> If so, then this patch can be a bit simpler, we can probably check `-` and always read value as unsigned int64.
>  I.e. both:
>
> - [-18446744073709551615, 18446744073709551615]
> - [       -0xFFFFFFFFFFFFFFFF,        0xFFFFFFFFFFFFFFFF]
>
>   ``` StringRef ScalarTraits<ELFYAML::YAMLInt>::input(StringRef Scalar, void *Ctx, ELFYAML::YAMLInt &Val) { const bool Is64 = static_cast<ELFYAML::Object *>(Ctx)->Header.Class == ELFYAML::ELF_ELFCLASS(ELF::ELFCLASS64); StringRef ErrMsg = "invalid number";
>
>   bool IsNegative = Scalar.front() == '-'; if (IsNegative) Scalar = Scalar.drop_front();
>
>   unsigned long long UInt; if (getAsUnsignedInteger(Scalar, /*Radix=*/0, UInt)) return ErrMsg;
>
>   // For a 32-bit target we allow values in a range of a uint32_t. if (!Is64 && (UInt > UINT32_MAX)) return ErrMsg;
>
>   Val = IsNegative ? (-UInt) : UInt; return ""; } ```
>
>   Did you mean something like that?


Not looked at the code too critically, but that more or less looks like what I'd expect. In hex land, I expect a given hex number to correspond identically to the bytes written. In other words `0xffffffffffffffff` would be written with the byte values 0xff 0xff etc, and have an effective value of -1. I think therefore it's logical to cause a '-' sign to negate the number. In other words, `-0xffffffffffffffff` would end up as 1. If somebody instead writes a decimal number, I think it can just be written as you'd expect (i.e. it's positive or negative, depending on the presence of the '-' sign). The difficulty is what to do about overflow detection, I guess. To keep things simple, I think it's best to do what you suggest in the quoted post. If I'm not mistaken, it's what the "natural" overflow behaviour would be for C++, so probably isn't too surprising.


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

https://reviews.llvm.org/D75527