[PATCH] D80629: [yaml2obj] - Allocate the file space for SHT_NOBITS sections in some cases.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/test/tools/obj2yaml/ELF/program-headers.yaml:499-500
+    Flags:  [ SHF_WRITE, SHF_ALLOC ]
 ## Use a size that is larger than the file size.
-    Size:  0x00000000FFFFFFFF
+    ShSize: 0x00000000FFFFFFFF
   - Name:  .data.2
----------------
grimar wrote:
> jhenderson wrote:
> > Looking at this test, I'm not convinced this is the right fix for the test. My impression is that this test is supposed to be about how segments are created and their file and memory sizes calculated. By using `ShSize` instead of `Size`, we are effectively saying (in theory) "ignore the size of this section for segment calculations" when I think what is meant is to ignore it for the calculation of the file size.
> > 
> > I think we need to properly change this test to use a real `Size` value here, just much smaller than what was chosen previously. Alternatively, we probably don't need this set of tests (nobits + segments) here at all, since we have the new tests you added that highlighted the bug of not allocating file space for nobits sections.
> > My impression is that this test is supposed to be about how segments are created and their file and memory sizes calculated. 
> 
> This is obj2yaml test. It is about how segments are dumped. This change does not change the original behavior, which is described as 
> "Use a size that is larger than the file size.", i.e. it checks how we dump such situation.
> 
> > I think we need to properly change this test to use a real Size value here, just much smaller than what was chosen previously.
> 
> It just will break the original intention ("Use a size that is larger than the file size.").
Oh, I missed that this is an obj2yaml test and not a yaml2obj one, thanks.


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

https://reviews.llvm.org/D80629