Page MenuHomePhabricator

Create a new library called ObjectLayout, and move headers from Support to there.
ClosedPublic

Authored by zturner on Jun 2 2017, 11:01 AM.

Details

Summary

Based on the list discussion from the other day, this does the following:

  1. Moves Dwarf.h, ELF.h, COFF.h, Wasm.h, MachO.h, all corresponding .def files, and all xxxReloc folders out of Support and into ObjectLayout.
  2. Moves the file_magic structure and identify_magic functions from Support to ObjectLayout.
  3. Moves corresponding unit tests from SupportTests to a new unit test target named ObjectLayoutTests.
  4. Updates all relevant LLVMBuild.txt files.
  5. Updates all code in llvm, clang, lld, and lldb to use the new header locations.

Sorry this patch is so large, but there is really no good way around it.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Jun 2 2017, 11:01 AM
chandlerc edited edge metadata.Jun 2 2017, 11:44 AM

About the only thing I find questionable here is the Magic.h bit... And really only because it also has file magic for archives and bitcode. But that's probably OK.

However, I hate to say this, but ObjectLayout doesn't seem like the right name. ObjectFormat would be closer or ObjectFile or something.

About the only thing I find questionable here is the Magic.h bit... And really only because it also has file magic for archives and bitcode. But that's probably OK.

However, I hate to say this, but ObjectLayout doesn't seem like the right name. ObjectFormat would be closer or ObjectFile or something.

One idea would be to call this library BinaryFormat. That encompasses things that aren't necessarily object files.

Responding to blaikie since responding by email triggers moderation:

I'm still missing some of the motivation as to 'why', here.
If there are significant uses of a library that don't use most of it - yeah, i think there's merit in separating things out (eg: if lld, lldb, etc, use, say, the DWARF constants but not the rest of Support/ADT - yeah, it'd be nice if they didn't have to rebuild whenever SmallString changed, for example). (though even then, Ninja's pretty efficient about only rebuilding what it has to, so I'm not sure that even the library granularity changes much about the build performance here - header dependency could be something to care about there, for sure (fewer long chains of dependent headers, breaking a header into two if there's functionality that is used broadly next to functionality that has a narrow use, etc).
What's the goal/metric/benefit?

First, there is the intangible benefit. DWARF.h and WASM.h simply do not make logical sense in Support. This might seem like a minor point, but I put this into the "technical debt" category. By not enforcing strict guidelines of what goes into Support, we build up technical debt over time as a bunch of unrelated stuff piles into it. We can do better than this.

Second, libraries as an abstraction are virtually free, and the point of them is to group related things together. One could probably even argue they are better than free, as they have less than or equal to zero cost since at worst they do nothing, and at best they speed up your build and reduce your binary size.

Third, there is the practical benefit. The only LLVMBuild.txt files modified here are lib/IR and lib/Object.

Any tool that did not depend on one of those two libraries should now build faster and be smaller in binary size. I don't have a list of which tools satisfy that condition, however, but it's definitely non-empty (FileCheck, for example, is an obvious candidate).

About the only thing I find questionable here is the Magic.h bit... And really only because it also has file magic for archives and bitcode. But that's probably OK.

However, I hate to say this, but ObjectLayout doesn't seem like the right name. ObjectFormat would be closer or ObjectFile or something.

One idea would be to call this library BinaryFormat. That encompasses things that aren't necessarily object files.

I personally like it. Let's give others a chance to chime in before you go renaming everything though. =D

chapuni added a subscriber: chapuni.Jun 2 2017, 3:32 PM

Anybody else have thoughts on the name?

Since the name discussion seems winding down, review on the code changes themselves...

Generally, there are a lot of unrelated #include order changes. Would it be ok if you (or I) do a widespread clang-format run over the #include lines to get them sorted so that your patch doesn't change that much? I'd like to avoid reverts or build bot trouble due to include order at the same time as trying to land the new library. Remaining (minor) comments inline.

Anybody else have thoughts on the name?

Peter Collingborne mentioned on IRC he liked BinaryFormat as well. I suggest going with that for now. Ship it! (Worst case, we can rename it later if needed...)

lld/COFF/Driver.cpp
43–44 ↗(On Diff #101240)

If these aren't used, delete the include as well?

llvm/include/llvm/IR/DebugInfoMetadata.h
2645–2646 ↗(On Diff #101240)

weird comment formatting.

llvm/include/llvm/ObjectLayout/Magic.h
17 ↗(On Diff #101240)

This probably shouldn't be in the object namespace any more. And format seems confusing. I guess binary_format would be unambiguous, but yuck.

I would probably just put this in the llvm namespace, and come back through here with improved names for all the things within it.

llvm/tools/llvm-objdump/MachODump.cpp
2598 ↗(On Diff #101240)

Weird comment reformat...

llvm/unittests/ObjectLayout/CMakeLists.txt
2 ↗(On Diff #101240)

Doesn't this depend on Support? (But maybe that works w/o listing it here)

zturner added inline comments.Jun 5 2017, 7:16 PM
lld/COFF/Driver.cpp
43–44 ↗(On Diff #101240)

The include is still needed. This file already has using namespace llvm::object, and since the two names were moved into the object namespace, no other changes are needed to make this work.

llvm/include/llvm/ObjectLayout/Magic.h
17 ↗(On Diff #101240)

It was actually in llvm::sys::fs before, and I moved it to object because it made more sense. But I can also just put it in llvm as you suggest. Also not keen on making a new namespace just for two or three classes.

llvm/unittests/ObjectLayout/CMakeLists.txt
2 ↗(On Diff #101240)

Works fine without it, because the library's LLVMBuild.txt references Support, so this is automatically picked up.

Since the name discussion seems winding down, review on the code changes themselves...

Generally, there are a lot of unrelated #include order changes. Would it be ok if you (or I) do a widespread clang-format run over the #include lines to get them sorted so that your patch doesn't change that much? I'd like to avoid reverts or build bot trouble due to include order at the same time as trying to land the new library. Remaining (minor) comments inline.

Where are you seeing these? I didn't click all 300+ files, but I did click about 20 of them and the only order changes I see are because the new name has a different relative sort order than the old name. But nothing to do with the old names being unsorted. So an initial clang-format pass over the files wouldn't do anything.

If there are specific files you're seeing in that I didn't find, LMK.

Since the name discussion seems winding down, review on the code changes themselves...

Generally, there are a lot of unrelated #include order changes. Would it be ok if you (or I) do a widespread clang-format run over the #include lines to get them sorted so that your patch doesn't change that much? I'd like to avoid reverts or build bot trouble due to include order at the same time as trying to land the new library. Remaining (minor) comments inline.

Where are you seeing these? I didn't click all 300+ files, but I did click about 20 of them and the only order changes I see are because the new name has a different relative sort order than the old name. But nothing to do with the old names being unsorted. So an initial clang-format pass over the files wouldn't do anything.

If there are specific files you're seeing in that I didn't find, LMK.

There are. Let me try updating them quickly and a rebase should fix it right up.

If there are specific files you're seeing in that I didn't find, LMK.

There are. Let me try updating them quickly and a rebase should fix it right up.

Yeah, the include order was just chaos everywhere in LLVM. I hadn't run scripts in too long. I wrote new scripts that directly use clang-format and updated all the include orderings so this kind of patch should be trivial. You should be able to just resolve all the conflicts in one direction or the other regardless and then run clang-format again.

You may want to patch https://reviews.llvm.org/D33932 into clang-format to handle the unittests, but hopefully we'll get that landed ASAP.

zturner updated this revision to Diff 101581.Jun 6 2017, 9:43 AM

Updates:

  1. Renamed ObjectLayout -> BinaryFormat.
  2. Rebased on ToT including chandler's include order changes.
  3. Re-ran clang-format with a fresh build of clang-format that includes D33932 patched in.
  4. Moved the file magic stuff from the llvm::object namespace to the llvm namespace and updated references accordingly.
chandlerc accepted this revision.Jun 6 2017, 5:04 PM

LGTM, ship it! (and watch aall the world burn!)

This revision was automatically updated to reflect the committed changes.