[PATCH] D74764: [yaml2obj][WIP] - Automatically assign sh_addr for allocatable sections.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:527
+
+  if (!(SHeader.sh_flags & ELF::SHF_ALLOC))
+    return;
----------------
MaskRay wrote:
> Delete. I think non-SHF_ALLOC sections follow the same rule.
Not sure I follow. We should not assign sh_addr for non-allocatable sections. Isn't?
In LLD we just drop counter to 0:

```
void LinkerScript::assignOffsets(OutputSection *sec) {
  if (!(sec->flags & SHF_ALLOC))
     dot = 0;
```

GNU linkers do not set addresses to sh_addr for non-allocatable sections too.
Why should yaml2obj do different thing?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:531
+  LocationCounter =
+      alignTo(LocationCounter, SHeader.sh_addralign ? SHeader.sh_addralign : 1);
+  SHeader.sh_addr = LocationCounter;
----------------
MaskRay wrote:
> getValueOr
`getValueOr`, which is a method of `Optional<>`? There is no optional here. I am testing `sh_addralign` for 0 because
`alignTo` expects it to be `!=0`: 

```
inline uint64_t alignTo(uint64_t Value, uint64_t Align, uint64_t Skew = 0) {
  assert(Align != 0u && "Align can't be 0.");
...
```


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml:31
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x1000
+    Size:    0x3
----------------
MaskRay wrote:
> 0x1000 is fine but probably not the best.
> 
> If .text.any.addr is included by a PT_LOAD, one will expect `sh_offset%p_align = sh_addr%p_align` (a PT_LOAD's p_align is usually 0x1000 (maxpagesize) on x86).
> 
> One approach is to specify sh_addralign and set sh_addr to sh_addralign plus a multiple of p_align.
> 
> ```
>     Address: 0x1200
>     AddressAlign: 0x200
> ```
I do not think it is a problem here. yaml2obj do just what it do: produce an output from a given description.
This test just documents the behavior of assigning addresses. It does not have intention to create an
ELF that has no issues.

And so it probably does not make much sence to work hard and put `0x1200` and `AddressAlign` here.
At least because it works only for `.text*`. See, the offset of `.data*` is broken:

```
Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00     0   0  0
  [ 1] .text.any.addr    PROGBITS        0000000000001200 000200 000003 00   A 0   0 512
  [ 2] .text.shsize      PROGBITS        0000000000001203 000203 001234 00   A 0   0  0
  [ 3] .text.align       PROGBITS        0000000000001300 000300 000004 00   A 0   0 256
  [ 4] .data.any.addr    PROGBITS        0000000000002000 000304 000001 00   A 0   0  0
  [ 5] .data.after.fill  PROGBITS        0000000000002101 000405 000001 00   A 0   0  0
  [ 6] .data.return.back PROGBITS        0000000000001500 000406 000001 00   A 0   0  0
  [ 7] .data.return.back.foo PROGBITS    0000000000001501 000407 000000 00   A 0   0  0
  [ 8] .dynsym           DYNSYM          0000000000001508 000408 000018 18   A 9   1  8
  [ 9] .dynstr           STRTAB          0000000000001520 000420 000001 00   A 0   0  1
  [10] .strtab           STRTAB          0000000000000000 000421 000001 00 0   0  1
  [11] .shstrtab         STRTAB          0000000000000000 000422 000093 00 0   0  1
```


Yes, it is possible to adjust other sections too, add a few properties to make them follow the rule, but that requires adding
additional fields, and I am not sure I see how it is useful in the context of this test case.
I see why it can be bad though :) It would make the test case larger than it can be.



================
Comment at: llvm/test/tools/yaml2obj/ELF/section-address-assign.yaml:64
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+## Used to trigger creation of .dynsym and .dynstr.
----------------
MaskRay wrote:
> Add a non-SHF_ALLOC section (e.g. .rela.debug_info) to test this rule applies on non-SHF_ALLOC sections.
We have `.strtab` and `.shstrtab` added implicitly. They are non-SHF_ALLOC.
I think I just do not understand what exactly you want me to test for them?


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

https://reviews.llvm.org/D74764