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

Sat Mar 14 08:33:43 PDT 2020

MaskRay added a comment.

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

> In D75527#1921825 <https://reviews.llvm.org/D75527#1921825>, @MaskRay wrote:
>
> > > For an 64-bit object any hex/decimal addends in range [-(2^64 - 1), 2^64 - 1)] is accepted.
> > >  For an 64-bit object any hex/decimal addends in range [-(2^32 - 1), 2^32 - 1)] is accepted.
> >
> > My feeling is that the ranges should be:
> >  [-(2^63 - 1), 2^64 - 1)]
> >  [-(2^31 - 1), 2^32 - 1)]
> >
> > > It should probably accept the union of int32_t and uint32_t, i.e. [-2147483648, 4294967295]. Examples: R_AARCH64_ABS32, R_AARCH64_PREL32, R_PPC64_ADDR32.
> >
> >
>
>
> But it does not work good for other relocations, isn't? Different relocations allows different ranges.
>  We are anyways not 100% correct with such approach until we don't handle each possible relocation individually.

I agree. So we just take the union of the use cases of all relocation types, but the range [-0x80000000, 0xffffffff] does not have to be larger.

> Currently (this diff), `YAMLInt` is kind of universal type. It can be used not only for addends, but as a
>  type or a base for type for other keys probably.
> 
>> For example, for an ELFCLASS32 object, an addend of -0xffffffff or -0x80000001 is invalid.
> 
> I have to say about what I think about negative hex numbers again, sorry.
>  Negitive hex numbers in YAML is something I suggested to ban in the first version of this diff.
> 
> I can read `-0xffffffff` as `--1` or as `-4294967296` or as a `-0x00000000ffffffff` at the same time.
>  But why actually we want to allow users to do this and think about values that hard?
>  Who will use negative hex numbers in a YAML tests and for what? I think it accepts less readable
>  tests and probably gives no any benefits. I think we should not focus too much on them (or just ban).
> 
> Speaking about decimal version of `-0xffffffff`: I have a test in this diff:
> 
>   ## Addend == -(2^32 - 1)
>   # RUN: yaml2obj --docnum=2 %s -o %t32.decimal.min -DADDEND=-4294967295
>   # RUN: llvm-readobj -r %t32.decimal.min | FileCheck %s --check-prefix=TEST -DADDEND=0x1
>   # RUN: yaml2obj --docnum=2 %s -o %t32.hex.min -DADDEND=-0xFFFFFFFF
>   # RUN: llvm-readobj -r %t32.hex.min | FileCheck %s --check-prefix=TEST -DADDEND=0x1
> 
> 
> `-0xffffffff` is accepted for 32-bits platform and reads the same as `-4294967295`, i.e. as `0x1`.
> 
> While I understand why you think that `-0xffffffff`` should be restricted in this case,
>  the current behavior looks acceptable for me in general.
> 
> I need this patch for D75671 <https://reviews.llvm.org/D75671> which wants to do `Addend: 0xffffffffffffffff`
>  for example and fails:
> 
>   YAML:17:17: error: invalid number
>           Addend: 0xffffffffffffffff
>                   ^~~~~~~~~~~~~~~~~~
>   yaml2obj: error: failed to parse YAML input: Invalid argument
> 
> 
> This is a problem I am trying to solve first of all.

I do take inspiration from lld's checkIntUInt: D14957 <https://reviews.llvm.org/D14957> D45051 <https://reviews.llvm.org/D45051> D63690 <https://reviews.llvm.org/D63690>.

> For example, for an ELFCLASS32 object, an addend of -0xffffffff or -0x80000001 is invalid.

I don't differentiate a decimal and a hexadecimal. Put it in another way, I think an addend of -4294967295 or -2147483649 is invalid. An addend is used to represent an address relative to a symbol or the load base by an offset. The offset can be as small as -2147483648 but it cannot be smaller than that.

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

https://reviews.llvm.org/D75527