[llvm] [llvm][yaml2obj] Modify section header overriding timing (PR #130942)

[James Henderson via llvm-commits]

https://github.com/jh7370 commented:

Unfortunately, apart from maybe a couple of quick responses over the next couple of hours, I've run out of time for this today and am off work for the next couple of weeks, so it'll be ~3 weeks before I can continue looking at this. Hopefully @MaskRay might be able to review this PR further. I'm happy for it to go in (once the issues have been addressed) if he is.

> We can see the test of `tools/yaml2obj/ELF/program-header-size-offset.yaml` [failed](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml#L34) because of the unmatched `FileSize` and `MemSize` of the last PorgramHeader after delaying the `overrideFields()`.
> 
> But the code here([FileSize](https://github.com/llvm/llvm-project/blob/main/llvm/lib/ObjectYAML/ELFEmitter.cpp#L1192) and [MemSize](https://github.com/llvm/llvm-project/blob/main/llvm/lib/ObjectYAML/ELFEmitter.cpp#L1206)) this behavior is correct. Both the last two section of the ProgramHeader is `SHT_NOBITS`, which won't occupy physical space in a file. Does this indicate that the current test has a mistake?

I _think_ the test is just using `ShOffset` when it should be using `Offset` instead. I'm not 100% certain about this though and would like @MaskRay to take a look if he gets a chance. Does changing that help fix any issues?

> (Additionally, the code only checks for the last `SHT_NOBITS` here, rather than all of them.)

It's too long ago to remember whether this was by design or not. I'd avoid changing it unless you can see there's an obvious hole in the test coverage of the actual code. @MaskRay may have a different opinion and I'm happy to defer to that.

> And I also found another two failed tests
> 
>     * [`tools/llvm-readobj/ELF/malformed-pt-dynamic.test`](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/malformed-pt-dynamic.test#L43): Failed because it assumes `p_offset` is determined by `sh_offset`.

I think the fix here is to avoid this assumption somehow. The test itself is fine, it's just the change in yaml2obj behaviour means we need a different way of forcing the p_offset value. I believe you can fix this test by removing the ShOffset field and replacing it with an Offset field in the program header description.

>     * [`tools/obj2yaml/ELF/program-headers.yaml`](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/obj2yaml/ELF/program-headers.yaml#L437): Failed due to a mismatch between the overridden `sh_offset` and the corresponding `p_offset`.

The line number reference here didn't make sense to me for the description you've mentioned. Are you referring to the bar/zed sections at https://github.com/llvm/llvm-project/blob/d0188ebcc206e334d5a415992e2b226c216b4083/llvm/test/tools/obj2yaml/ELF/program-headers.yaml#L570 and the check at line 533? If so, I'm not sure what the fix should be here. Changing `ShOffset` to `Offset` in the zed section description might work, but I'm not sure if it will or not (it might result in a different error). If that doesn't work, you might be able to use the [SectionHeaderTable field](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/yaml2obj/ELF/section-headers.yaml) in the YAML to reorder the section headers: put the zed section first in the Sections: list in the YAML, without any specific offset (but a non-zero size), followed by bar, but then in the SectionHeaderTable listing, specify the sections in the opposite order. You might need to break the individual test case into two parts, with two different YAMLs, each for one of the warning messages. Aside: this test case is in the wrong directory, as it's purely a yaml2obj test, not an obj2yaml one. I'm not sure it's worth moving though.

https://github.com/llvm/llvm-project/pull/130942