Page MenuHomePhabricator

[test/Object, obj2yaml] - Move test cases from test/Object and cleanup.
ClosedPublic

Authored by grimar on Jul 11 2019, 4:03 AM.

Details

Summary

test/Object is not correct place to have tests that check obj2yaml
functionality, because we have test/tools/obj2yaml folder for that.

In this patch I merged a few test cases with their YAMLs from Inputs
folder, converted one of binary inputs and moved them to
tools/obj2yaml folder.

There are still another tests that might need the same, so it is initial step.
My motivation was that I was able to damage a code responding for dumping/reading
group sections and no tests in yaml2obj/ob2yaml failed, what was really
confusing to see.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 11 2019, 4:03 AM
grimar edited the summary of this revision. (Show Details)
jhenderson added inline comments.Jul 11 2019, 4:34 AM
test/tools/obj2yaml/invalid-reloc.test
1 ↗(On Diff #209161)

What exactly is broken about the relocation? It's valid for some kinds of relocations to have no symbol.

I think this test needs renaming and the comment updating.

test/tools/obj2yaml/section-group.test
43 ↗(On Diff #209161)

Nit: too many spaces between tag and value.

46 ↗(On Diff #209161)

Can you get rid of the two section symbols?

I think there are trailing spaces in these YAML files (e.g. in Object/Inputs/COFF/section-aux-symbol.yaml, header: is followed by 10 spaces). When you commit the patch, it might to good to delete them :) D64566

MaskRay added inline comments.Jul 11 2019, 7:14 AM
test/tools/obj2yaml/invalid-reloc.test
1 ↗(On Diff #209161)

asan instrumentation may generate R_*_NONE for debug info. ld -r may generate R_*_NONE. They are used as placeholders when no reasonable relocation is available. In this sense, they are indeed "broken".

However, here it is probably better to say that a symbol index of 0 can be dumped...

grimar marked an inline comment as done.Jul 11 2019, 7:39 AM
grimar added inline comments.
test/tools/obj2yaml/invalid-reloc.test
1 ↗(On Diff #209161)

Yeah. I based my comment on the initial comment (rL293224):

R_X86_64_NONE can be emitted without a symbol associated (well,
in theory it should never be emitted in an ABI-compliant relocatable
object). So, if there's no symbol associated to a reloc, emit one
with an empty name, instead of crashing.

I'll rephrase.

grimar updated this revision to Diff 209448.Jul 12 2019, 3:16 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
test/tools/obj2yaml/section-group.test
46 ↗(On Diff #209161)

Done.

This revision is now accepted and ready to land.Jul 12 2019, 3:18 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 3:30 AM