This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] Move core yaml2obj code into lib and include for use in unit tests
ClosedPublic

Authored by abrachet on Jul 24 2019, 4:33 PM.

Diff Detail

Event Timeline

abrachet created this revision.Jul 24 2019, 4:33 PM
abrachet updated this revision to Diff 211665.Jul 24 2019, 9:01 PM

Added StringRef include

Thanks for taking this on. I look forward to being able to use this in lldb tests.

I'm not an owner here, but the main question I have is about the library-readiness of the code you're moving. I see it's doing things like spewing errors to stderr and even calling exit(), neither of which is a very nice thing to do for a library (even if it's just a "test" library). Do you have any plans for addressing that?

llvm/unittests/ObjectYAML/yaml2ObjectFileTest.cpp
30 ↗(On Diff #211665)

nit: ASSERT_THAT_EXPECTED (from Testing/Support/Error.h) will print the actual error message if this ever fails (instead of just a useless "false is not true").

Generally looks OK to me. Comments/questions are inlined.

llvm/lib/ObjectYAML/yaml2obj.cpp
32

yaml2obj was a tool name. Should it still be printed?

Also seems it should not refer to file anymore, should it be "failed to parte YAML input"?
I guess that worth a unit test probably.

MaskRay added inline comments.Jul 25 2019, 1:32 AM
llvm/lib/ObjectYAML/WasmEmitter.cpp
658–661

It is probably time to wrap these things in

namespace llvm {
namespace yaml {
...
}
}

Thanks for taking this on. I look forward to being able to use this in lldb tests.

I'm not an owner here, but the main question I have is about the library-readiness of the code you're moving. I see it's doing things like spewing errors to stderr and even calling exit(), neither of which is a very nice thing to do for a library (even if it's just a "test" library). Do you have any plans for addressing that?

I suggested to Alex offline that he not try to do any more than the bare minimum to get this moved over. Certainly more work needs doing to it, but I think that can be done at a later point rather than upfront when moving it, given how useful it will be to have when working on the libObject code if nothing else.

llvm/include/llvm/ObjectYAML/yaml2obj.h
53

Let's have this return an llvm::Error. I shouldn't massively increase the complexity of this code.

llvm/lib/ObjectYAML/CMakeLists.txt
8

Looks like this CMake is missing any reference to the include files, which means they won't get auto-magically added to things like Visual Studio's projects. If you take a look at libObject's CMakeLists.txt, you'll find an example of what to do.

29–34

My instinct is that we should rename these files to match the naming style of other files in this library. Open to other suggestions though.

llvm/lib/ObjectYAML/yaml2obj.cpp
20–23

We should avoid functions like this in the new library code. Better would be something that returned llvm::Error. In practice, you should probably just call createStringError at all its call sites and pass the resultant Error up the stack.

llvm/unittests/ObjectYAML/CMakeLists.txt
10

Looks like filenames usually start with an upper-case letter. Also, the filenames usually reflect the names of the file they are testing, in this case yaml2obj.cpp, so let's call this YAML2ObjTest.cpp.

Thanks for taking this on. I look forward to being able to use this in lldb tests.

I'm not an owner here, but the main question I have is about the library-readiness of the code you're moving. I see it's doing things like spewing errors to stderr and even calling exit(), neither of which is a very nice thing to do for a library (even if it's just a "test" library). Do you have any plans for addressing that?

I suggested to Alex offline that he not try to do any more than the bare minimum to get this moved over. Certainly more work needs doing to it, but I think that can be done at a later point rather than upfront when moving it, given how useful it will be to have when working on the libObject code if nothing else.

I agree. It'd be useful if @labath can elaborate which parts he intends to use in lldb. This will help decide how much of yaml2obj should be moved to lib/ObjectYAML.

llvm/tools/yaml2obj/yaml2obj.cpp
45

There is probably not much we gain by moving this part to lib/ObjectYAML.

abrachet updated this revision to Diff 211876.Jul 25 2019, 9:02 PM

Changed return of convertYAML to Error. Changed name of files from yaml2X to XEmitter. Wrapped unexported functions in anonymous namespace. clang-format.

abrachet marked 9 inline comments as done.Jul 25 2019, 9:09 PM

git clang-format does not understand moved files it turns out, so it formatted things that I didn't touch. I figure if this is going to happen it might as well be now, though. I can change this back though.

llvm/lib/ObjectYAML/CMakeLists.txt
8

CMake doesn't fail when generating files so I haven't done anything horribly wrong. I assume this works but I don't know how to test if it did.

llvm/lib/ObjectYAML/WasmEmitter.cpp
658–661

I did this in all the files. Also I wrapped things in an anonymous namespace, is this ok?

llvm/tools/yaml2obj/yaml2obj.cpp
45

I think its more convenient to not have to specify the file type. I wonder what others have to say.

abrachet marked 5 inline comments as done.Jul 25 2019, 9:10 PM
abrachet added inline comments.
llvm/unittests/ObjectYAML/yaml2ObjectFileTest.cpp
30 ↗(On Diff #211665)

Thanks! I've been doing this a lot great to know about this better way!

I suggested to Alex offline that he not try to do any more than the bare minimum to get this moved over. Certainly more work needs doing to it, but I think that can be done at a later point rather than upfront when moving it, given how useful it will be to have when working on the libObject code if nothing else.

I agree. It'd be useful if @labath can elaborate which parts he intends to use in lldb. This will help decide how much of yaml2obj should be moved to lib/ObjectYAML.

I'd like have a function which takes a (yaml) string, and get raw bytes in return. So, that would be slightly less that what you're doing in the unit test here (as you're also re-parsing those bytes into an object file), but I am guessing you're going to need the re-parsing bits for the work you're doing anyway.

Also, due to how lldb's object parsers work, I'll need to save that stream of bytes into a file, but that is something that can be easily handled on the lldb side too.

As for object types, my main interest is ELF files, as we already have unit tests using those (by shelling out to yaml2obj). However, having COFF and MachO support would be nice too, and if it's available people might be inclined to use it. Minidump is interesting too, but I already have a mechanism for using that.

I don't know if that answers your question. If it doesn't, you'll have to ask me something more specific. :)

llvm/lib/ObjectYAML/WasmEmitter.cpp
658–661

llvm policy http://llvm.org/docs/CodingStandards.html#anonymous-namespaces is to keep the anonymous namespaces as small as possible, preferring the static keyword instead. So, in this case, that would mean only the class definitions need to be wrapped in namespace{}.

llvm/tools/yaml2obj/yaml2obj.cpp
45

I'm pretty ambivalent. On one hand, it's nice to have the same interface as the command line tool, but on the other, in the test you should always know the object type, so appending that to the function name does not hurt.

jhenderson added inline comments.Jul 26 2019, 4:51 AM
llvm/lib/ObjectYAML/CMakeLists.txt
8

I checked with equivalent code, and it did the job on my Windows machine.

llvm/lib/ObjectYAML/yaml2obj.cpp
21

Maybe mark this with TODO:

Rather than an empty string, let's at least report something like "yaml2obj failed".

23

You don't need the trailing return type, I'm pretty sure.

llvm/test/tools/yaml2obj/invalid-docnum.test
23

Nit: no new line at end of file.

abrachet updated this revision to Diff 211998.Jul 26 2019, 1:41 PM
abrachet marked an inline comment as done.

Addressed review comments.

abrachet marked 7 inline comments as done.Jul 26 2019, 1:58 PM
abrachet added inline comments.
llvm/lib/ObjectYAML/yaml2obj.cpp
23

For the first time, this one actually needs it :) Error::success() returns an ErrorSuccess

llvm/tools/yaml2obj/yaml2obj.cpp
45

I think the alternative would be code used in the same way in every test which wants to use it. See D64281 for how it can be used in unit tests. That was the same pattern that @labath did here D59482. It's not just about explicitly stating the object file type, this also does error handling etc.

jhenderson added inline comments.Jul 29 2019, 8:06 AM
llvm/lib/ObjectYAML/COFFEmitter.cpp
36

This struct should be in the anonymous namespace.

llvm/lib/ObjectYAML/ELFEmitter.cpp
57–58

You're closing the anonymous namespace here, but start one again about 3 lines later. Merge them together.

llvm/lib/ObjectYAML/WasmEmitter.cpp
26

This class should be in the anonymous namespace.

llvm/test/tools/yaml2obj/invalid-docnum.test
2

This sentence is poor grammar. Probably something like "Test that an error is reported when a docnum is specified, which is greater than the number of YAML inputs in the file."

abrachet updated this revision to Diff 212230.Jul 29 2019, 2:37 PM
abrachet marked an inline comment as done.
abrachet retitled this revision from [yaml2obj] Move core yaml2obj code into lib and include for use in unittests to [yaml2obj] Move core yaml2obj code into lib and include for use in unit tests.

Addressed review comments

abrachet marked 4 inline comments as done.Jul 29 2019, 2:37 PM

git clang-format does not understand moved files it turns out, so it formatted things that I didn't touch. I figure if this is going to happen it might as well be now, though. I can change this back though.

I think it's fine to clang format the whole thing (since git clang-format isn't smart enough), but ideally it'd be a separate patch that lands first, so that this just shows the changes required for moving directories

llvm/lib/ObjectYAML/CMakeLists.txt
8

This fails in a -DBUILD_SHARED_LIBS=On cmake build; I think you need to add a dep on MC here (or add set(LLVM_LINK_COMPONENTS MC))

FAILED: lib/libLLVMObjectYAML.so.10svn
...
ld.lld: error: undefined symbol: llvm::StringTableBuilder::StringTableBuilder(llvm::StringTableBuilder::Kind, unsigned int)                                                                                       
>>> referenced by ELFEmitter.cpp:104 (/usr/local/google/home/rupprecht/src/llvm-project/llvm/lib/ObjectYAML/ELFEmitter.cpp:104)                                                                                   
>>>               lib/ObjectYAML/CMakeFiles/LLVMObjectYAML.dir/ELFEmitter.cpp.o:(llvm::yaml::yaml2elf(llvm::ELFYAML::Object&, llvm::raw_ostream&))
abrachet updated this revision to Diff 212514.Jul 31 2019, 12:27 AM

fixed link issue

abrachet marked 2 inline comments as done.Jul 31 2019, 12:28 AM
abrachet added inline comments.
llvm/lib/ObjectYAML/CMakeLists.txt
8

I ended up having to add quite a few more libraries before it would link. Were you just getting the errors you showed here or more than this?

rupprecht added inline comments.Jul 31 2019, 10:40 AM
llvm/lib/ObjectYAML/CMakeLists.txt
8

Oh, sorry, this was the wrong suggestion -- the link libraries are controlled by the other build system, LLVMBuild.txt (see http://llvm.org/docs/LLVMBuild.html).
MC is the only one that needed to be added, but by setting LLVM_LINK_COMPONENTS, that appears to overwrite what's in LLVMBuild.txt, leading to even more link errors.

Can you undo the change here and add MC to LLVMBuild.txt?

abrachet updated this revision to Diff 212677.Jul 31 2019, 3:27 PM
abrachet marked an inline comment as done.

Fixed link error in LLVMBuild.txt not CMakeLists.txt

labath added inline comments.Jul 31 2019, 11:26 PM
llvm/lib/ObjectYAML/CMakeLists.txt
8

Would this mean that we can also remove some entries from LLVM_LINK_COMPONENTS for the yaml2obj tool itself, now that it does not reference some of these libraries directly?

abrachet updated this revision to Diff 212738.Aug 1 2019, 12:12 AM

Removed unnecessary LLVM_LINK_COMPONENTS in yaml2obj

abrachet marked 3 inline comments as done.Aug 1 2019, 12:14 AM
abrachet added inline comments.
llvm/lib/ObjectYAML/CMakeLists.txt
8

Thank you @rupprecht that worked :)

Oh yeah. Thanks @labath :)

rupprecht accepted this revision.Aug 1 2019, 9:52 AM
This revision is now accepted and ready to land.Aug 1 2019, 9:52 AM
seiya added a subscriber: seiya.Aug 2 2019, 1:11 AM
This revision was automatically updated to reflect the committed changes.
abrachet marked an inline comment as done.

Sorry, this patch broke tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/15993, I reverted it in r368035.

I fixed those here rG9eee4254796df1a34a0452fa91e8ce4e38b6a5bb. Could you re-land or do I need to do it?

abrachet updated this revision to Diff 213715.Aug 6 2019, 2:01 PM

Fixed failing test cases

abrachet reopened this revision.Aug 6 2019, 6:36 PM
This revision is now accepted and ready to land.Aug 6 2019, 6:36 PM
This revision was automatically updated to reflect the committed changes.