[PATCH] D62809: [yaml2obj] - Change how we handle implicit sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:405
+  SHeader.sh_entsize = (DocSec && DocSec->EntSize)
+                           ? (unsigned)(*DocSec->EntSize)
+                           : sizeof(Elf_Sym);
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > Casting to `unsigned` here seems wrong, since entsize is a 64 bit number...
> > > Hmm, right.
> > Actually, I lied slightly, in that sh_entsize's size is dependent on the ELF format (32 versus 64 bits). Not sure what the right approach here is though. We could truncate (as I think this will do) or emit an error (but the latter might be a little annoying to implement. Truncation is probably fine for now.
> > Actually, I lied slightly, in that sh_entsize's size is dependent on the ELF format (32 versus 64 bits). 
> 
> Yes, I know, but I think we always truncate values everywhere.
> We have a comment saying "Just use 64, since it can hold 32-bit values too." in ELFYAML.h where we declare types for ELF things. And I do not think I saw any code checking we do not truncate, so I also guess it is fine for now.
> 
> (Anyways., if we want to fix it, it is probably should be checked much earlier. I.e. we might want to report an overflow error during parsing YAML on 32 bits platforms probably).
Yes, you're probably right.


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

https://reviews.llvm.org/D62809