[PATCH] D78927: [yaml2obj] - Introduce the "Offset" property for sections.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:48
-    return OS;
-  }
 
----------------
grimar wrote:
> FTR, after commiting this patch I faced LLD test failture and fixed it:
> https://github.com/llvm/llvm-project/commit/969c63a2ecfb536062ff2174645abe31e4036067
> (it started to report "section [index 1] has a sh_offset (0x1000000000000000) + sh_size (0x4) that is greater than the file size (0x100000120)" instead of "section sh_addralign is too large" LLD's error)
> 
> The issue was related to "AddressAlign:    0x1000000000000000" line in the YAML:
> 
> ```
>   - Name:            .text
>     Type:            SHT_PROGBITS
>     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
>     AddressAlign:    0x1000000000000000
>     Content:         "00000000"
> ```
> 
> 
> Previously, we had this piece of code which used `unsigned`, so `AddressAlign` was truncated.
> As a result a little binary with `sh_addrallign==0x1000000000000000` was produced.
> After this patch, this piece does not exist and currently for that test a 500mb binary with with `sh_addrallign==0x1000000000000000` is produced. I.e. it works a bit more correct, but
> the binary is is still truncated.
> 
> It happens because of `ELFState<ELFT>::alignToOffset`, which has the following lines:
> 
> ```
>   CBA.getOS().write_zeros(AlignedOffset - CurrentOffset);
>   return AlignedOffset;
> ```
> 
> `AlignedOffset`  and `CurrectOffset` are `uint64_t`, but `raw_ostream::write_zeros` takes `unsigned`:
> 
> ```
> /// indent - Insert 'NumSpaces' spaces.
> raw_ostream &raw_ostream::indent(unsigned NumSpaces) {
>   return write_padding<' '>(*this, NumSpaces);
> }
> 
> /// write_zeros - Insert 'NumZeros' nulls.
> raw_ostream &raw_ostream::write_zeros(unsigned NumZeros) {
>   return write_padding<'\0'>(*this, NumZeros);
> }
> ```
> 
> Probably we want to change it to take `uint64_t`.
> I do not think there is a way to write a test for it though as the result output would be huge.
Ind I do not believe someone really needs `uint64_t` there to put so many zeroes, so I am not sure if we want to change it for "correctness" or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78927