This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] Add support for load config section data.
ClosedPublic

Authored by jacek on Apr 28 2023, 6:03 AM.

Details

Summary

This makes testing load config much nicer and will allow more readable tests in D149091. Currently llvm-readobj uses binary blobs in tests, with this patch we could rewrite them to use yaml2obj instead.

Diff Detail

Event Timeline

jacek created this revision.Apr 28 2023, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 6:03 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jacek requested review of this revision.Apr 28 2023, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 6:03 AM
jacek updated this revision to Diff 518464.May 1 2023, 8:54 AM

A new version with yaml2obj.rst changes.

jacek updated this revision to Diff 527409.Jun 1 2023, 7:13 AM

Rebased on top of D149439 changes.

jacek updated this revision to Diff 536243.Jun 30 2023, 7:41 AM

Updated with additional use in arm64ec-chpe.yaml.

I'd recommend you have test cases where all the fields are present and none of them are. For the purposes of yaml2obj testing, it doesn't actually need to make sense as an output file!

lld/COFF/Config.h
107 ↗(On Diff #536243)

Unrelated? Perhaps should be a follow-up change...

llvm/lib/ObjectYAML/COFFEmitter.cpp
608

You should have a test case for the behaviours in this function.

Oh, looks like clang-format is failing in pre-merge. Please make sure you've formatted the new code you've added.

jacek updated this revision to Diff 536896.Jul 3 2023, 2:19 PM

Rebased on top of D149439 changes, added more tests and removed unrelated change. Thanks for review!

jhenderson added inline comments.Jul 4 2023, 12:02 AM
llvm/lib/ObjectYAML/COFFEmitter.cpp
606

Having now reread this bit of code, I now see that S.Size is being cast to size_t, whereas I misread it originally as being a sizeof due ot the similarity it has with the sizeof(S) earlier in the line. Perhaps a static_cast would be more obvious?

I'm assuming that S.Size corresponds to the Size field in the YAML. Assuming that is the case, what happens if a load config member would be written AFTER the end of a truncated block (i.e. where S.Size is less than sizeof(S))? Should this have a distinct test case?

llvm/test/tools/yaml2obj/COFF/load-config-empty.yaml
32 ↗(On Diff #536896)

Is this equivalent to having a Size: 0 case? If so, it might be worth being explicit about it. If not, it might be worth having that test case too.

llvm/test/tools/yaml2obj/COFF/load-config32-large.yaml
1 ↗(On Diff #536896)

It would be useful having an explanatory comment at the top of each of these tests briefly outlining what they are specifically testing.

jacek updated this revision to Diff 537186.Jul 4 2023, 3:49 PM

Rebased, changed size_t casts, added more offset checks and more tests.

For the context: load config size grows over time. Microsoft occasionally extends it with new fields appended to the end. To manifest the version used, users set load config's first field, Size, to the size of variant of the structure that they use. The handling that I wrote is meant to allow specifying various versions of the struct.

With the previous version, members that did not fit specified size were silently ignored. The new version checks for that, so that it will be considered an invalid yaml, it may help catch mistakes.

jhenderson added inline comments.Jul 5 2023, 2:00 AM
llvm/lib/ObjectYAML/COFFYAML.cpp
565–566

Are you sure that wants to be sizeof(S.Size) in these two lines? Would it be worth giving S a more descriptive name?

llvm/test/tools/yaml2obj/COFF/load-config-empty.yaml
1 ↗(On Diff #537186)

Nit: in newer tools tests, we tend to use ## for comments, to help distinguish them from RUN/CHECK lines etc. Same throughout.

llvm/test/tools/yaml2obj/COFF/load-config-large.yaml
1 ↗(On Diff #537186)

I'm beginning to think it might be worth grouping all these tests into a single test file, with multiple YAMLs (using yaml2obj --docnum to pick the relevant YAML). This is because the behaviour is all closely related to each other, and having the tests in the same file would therefore make it easier to find all tests to do with a particular feature.

jacek added inline comments.Jul 5 2023, 5:34 AM
llvm/lib/ObjectYAML/COFFYAML.cpp
565–566

Yes, that's what I meant. Size is the first member of load config structs. Load config that's too small to even fit the size field itself wouldn't make much sense. I will add a comment to clarify that, but I can also remove the check if you prefer (the check is not required from yaml2obj's point of view, I added it for completeness, it has a chance to catch some user errors).

jacek updated this revision to Diff 537320.Jul 5 2023, 5:43 AM
jacek marked 5 inline comments as done.

Moved tests to a single file, added comments, renamed load config struct.

jacek marked an inline comment as done.Jul 5 2023, 5:46 AM
jhenderson added inline comments.Jul 6 2023, 12:37 AM
llvm/lib/ObjectYAML/COFFYAML.cpp
559
565–566

In other areas of yaml2obj (at least in the ELF side), we try to be as permissable as possible, as long as the object could be reasonably representable in the file format. For example, you could have an ELF symbol table without any symbols at all, even though the ELF file format states that there must be at least one symbol at index 0. So in this case, it might be reasonable to allow a zero-sized LoadConfig structure, so that you could test that consumer tools (e.g. llvm-readobj) can handle such a situation (even if it's just a case of reporting an error).

TL;DR: I don't really mind what you do. It largely depends how you want to use the tool.

llvm/test/tools/yaml2obj/COFF/load-config.yaml
1 ↗(On Diff #537320)

With multi-case tests like this, if they aren't sharing the same checks/inputs, I tend to write them like this:

## Test case comment.
# RUN: ...
# RUN: ... | FileChcek %s --check-prefix=FOO

# FOO: ...

<yaml for test case 1>

## Test case 2 comment.
# RUN: ...
# RUN: ... | FileChcek %s --check-prefix=BOO

# BOO: ...

<yaml for test case 2>

etc, as this makes it easier to see the parts of the respective test case. OTOH, if the inputs are shared between many test cases, possibly through the use of yaml2obj's -D option, I tend to put the input at the end of the block of test caes that use it.

jacek marked an inline comment as done.Jul 6 2023, 4:32 AM
jacek added inline comments.
llvm/lib/ObjectYAML/COFFYAML.cpp
565–566

For sizes like 2 I think it would be cleaner to use '-Binary: 0200' instead of LoadConfig and there is not much that llvm-readobj can do about such config: it will try to interpret bytes following load config as part of the size field. My opinion is not strong, but it felt right to have the check if we also have size checks for other members, so since you don't mind, I will leave it.

jacek updated this revision to Diff 537663.Jul 6 2023, 4:34 AM

Reorganized test content.

jacek updated this revision to Diff 538339.Jul 8 2023, 2:33 AM

I misread your suggestion, sorry. The attached version moves yaml contents as well.

MaskRay added inline comments.Jul 23 2023, 4:45 PM
llvm/test/tools/yaml2obj/COFF/load-config.yaml
10 ↗(On Diff #538339)

I think we generally add -NEXT. When something goes off, FileCheck will give a better error. This does mean some maintenance overhead when adding a new field, but in general the advantage outweighs the downside.

jhenderson accepted this revision.Jul 24 2023, 1:04 AM

LGTM, with two nits and MaskRay's comment addressed.

llvm/lib/ObjectYAML/COFFYAML.cpp
559

Ping this comment.

llvm/test/tools/yaml2obj/COFF/load-config.yaml
237 ↗(On Diff #538339)

Nit

This revision is now accepted and ready to land.Jul 24 2023, 1:04 AM
jacek updated this revision to Diff 543650.Jul 24 2023, 11:07 AM
jacek marked 4 inline comments as done.

Thanks for reviews.

This revision was landed with ongoing or failed builds.Jul 24 2023, 11:42 AM
This revision was automatically updated to reflect the committed changes.