[LLVM][llvm-objcopy] Added basic plumbing to get things started

[jakehehrlich]

Summary

As discussed on llvm-dev I've implemented the first basic steps towards llvm-objcopy/llvm-objtool (name pending).

This change adds the ability to copy (without modification) 64-bit little endian ELF executables that have SHT_PROGBITS, SHT_NOBITS, SHT_NULL and SHT_STRTAB sections.

[jakehehrlich]

I added ELFT support. This demanded making a number of structural changes to how I was doing things. I decided 'initialize' no longer made sense. I also decided it was best to construct special sections like the section names table independently of reading the file. Also I previously wasn't aligning anything. I added some basic alignment support so that sections and segments are properly aligned.

[grimar]

Have few general comments.

test/tools/llvm-objcopy/basic-copy.test

3 ↗

(On Diff #102082)

I would add empty line here.

15 ↗

(On Diff #102082)

And here. Just to separate execution flow from check in a more obvious way.

tools/llvm-objcopy/Object.cpp

36

I would inline CompOffset, it is trivial operation that does not probably require addition comparator.

76

Count

121

Seg.Index = Index++;

189

A, B

198

LLVM code style says variable names should be uppercase. Value, Multiple, so on.

199

Is it clang-formatted ?

269

Consider using typedefs in header
(example from LLD code)

template <class ELFT> class Writer {
public:
  typedef typename ELFT::Shdr Elf_Shdr;
  typedef typename ELFT::Ehdr Elf_Ehdr;
  typedef typename ELFT::Phdr Elf_Phdr;

tools/llvm-objcopy/Object.h

43 ↗

(On Diff #102082)

Sort by Name may be ?

And looks you can do initialization inline instead of doing that in constructor below.

94 ↗

(On Diff #102082)

Consider using llvm::StringMap may be ?

104 ↗

(On Diff #102082)

I think you can inline initialization,

132 ↗

(On Diff #102082)

Not sure how much is in common not to include argument names in definitions.
(in LLD where I spend most of time I am working on, we always do that, but may be it is ok, just noticable thing for me).

tools/llvm-objcopy/llvm-objcopy.cpp

53 ↗

(On Diff #102082)

ELF64LE ? I guess you want to template it.

94 ↗

(On Diff #102082)

You either want to remove brackets here:

if (ELFObjectFile<ELF64LE> *o = dyn_cast<ELFObjectFile<ELF64LE>>(&Binary))
  CopyBinary(*o);
 else
  reportError(InputFilename, object_error::invalid_file_type);

Or probably better just to early return:

if (ELFObjectFile<ELF64LE> *o = dyn_cast<ELFObjectFile<ELF64LE>>(&Binary))
  return CopyBinary(*o);

reportError(InputFilename, object_error::invalid_file_type);
 return X;

[jakehehrlich]

Fixed things recommended by grimar

[jakehehrlich]

Changed it to use llvm::StringMap instead of std::map and also fixed the basic test.

[meadori]

I having looked through the fine details yet, but one higher level question: I saw the discussions on the mailing list concerning being careful not to go overboard making everything work with multiple object file formats and to start with ELF. That being said, why can't just the ELF functionality of libObject be extended to write? Seems like going that way would make it easier for others to reuse in other tools.

[jakehehrlich]

In D33964#781402, @meadori wrote:

I having looked through the fine details yet, but one higher level question: I saw the discussions on the mailing list concerning being careful not to go overboard making everything work with multiple object file formats and to start with ELF. That being said, why can't just the ELF functionality of libObject be extended to write? Seems like going that way would make it easier for others to reuse in other tools.

As I understand it libObject reads in the file to memory and then simply reinterpret_casts the different chunks of the file into the corresponding parts. So when you get an Elf64_Shdr* from the libObject interface you actually are getting a pointer into the file data. It's sort of like "in-place" file parsing. It might even be memory mapping the file, I'm not sure. My point is that It does not have it's own internal modifiable representation of the file. It just has the raw data from the file. Changing the interface to include section/symbol removal (which involves changing file layout and modifying sections like .shstrtab and .shtab) would cause the internal workings of libObject to change a lot I think. Perhaps someone can confirm my understanding?

[meadori]

Okay, that makes sense. Thanks to all that explained it.

With that in mind, is there any reason not to but the read/write object implementation that is started in this patch into a new library? ISTM, that functionality would be quite useful for various object file manipulation tools.

[jakehehrlich]

There are several llvm projects that use similar functionality so a library should be implemented at some point. Petr spoke with Rui about this possibility of de-duplication of efforts. The conclusion of this discussion was that we should build llvm-objcopy independently and think about de-duplication later. Sean Silva expressed a similar sentiment in the llvm-objcopy proposal email. I may be speaking out of turn here but I think everyone agrees that a) there is duplication occurring across llvm here and b) now is not the right time to tackle that issue but it should defiantly be in thought about. There is some concern over weather the de-duplication even makes sense. I think as llvm-objcopy starts to be fleshed out it will become clear what can be de-duplicated and what cannot. That will give us a good idea of what should and shouldn't go in this library.

[meadori]

Seeing how things grow and de-duplicating as necessary seems like a reasonable strategy to me.

[jakehehrlich]

ping/bump

[jakehehrlich]

After a bit offline discussion with Petr I fixed a couple things like formatting and error handling.

[phosek]

LGTM, but I'd like to have a second opinion.

[Bigcheese]

Thanks for working on this! It's been a low priority on my todo list for years now.

I'm a bit concerned about the section layout algorithm. It ignores the section offsets from the input sections and just computes new ones derived from the address and size. This will fail when given an ELF which has a gap in the sections in a given segment. I think the hardest part of objcopy will be making sure to keep the file in a consistent state, and I'd like to see a design that makes it hard to get wrong.

tools/llvm-objcopy/Object.cpp

171

Memory leak. Use unique_ptr.

193

Use alignTo from llvm/Support/MathExtras.h

224

Replace with alignTo?

tools/llvm-objcopy/Object.h

3 ↗

(On Diff #102218)

The LLVM Compiler Infrastructure

90 ↗

(On Diff #102218)

This should probably use llvm/MC/StringTableBuilder.h

100 ↗

(On Diff #102218)

dead code

104–106 ↗

(On Diff #102218)

dead code

[jakehehrlich]

1. Switched to using StringTableBuilder (which makes StringTableSection just a wrapper around StringTableBuilder).
2. I removed some dead code pointed out by BigCheese.
3. I switched to alignTo.
4. Corrected licence headers

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

171

It get's emplaced into the Sections vector first thing after allocation and that unique pointer manages the memory. Would be more obvious that a leak isn't occurring I first add a new unique pointer to Sections and then assign SectionNames via a cast from there?

[Bigcheese]

tools/llvm-objcopy/Object.cpp

171

Ah, I see now. It would probably be best to do:

Sections.push_back(make_unique<StringTableSection>());
SectionNames = Sections.back().get();
...

[jakehehrlich]

Made it obvious that a memory leak is not occurring as recommended by BigCheese.

[jakehehrlich]

In working on some more other features and improvements to this I realized that I had dangling pointer issue. I was assigning pointers to Segments out of an std::vector. These pointers would become invalid on resize. To fix this I simply used unique pointers to store the segments. The code changed very little as a result since I try my best not to mess with program headers.

[jhenderson]

Looking good so far to me, apart from lots of small nits for the most part.

One general nit on comments - I believe that they should all end in full stops.

tools/llvm-objcopy/Object.cpp

18

Could we typedef these as well? It will read more easily, I think.

24

Any particular reason this TODO has been left undone?

165

I personally would think that using offset to determine order may be simpler, as it removes the need to test for SHF_ALLOC. Also, sections don't have to appear in the file in the order that they appear in the section header table, so I'd only use Index as a fall-back if you get two sections with the same offset.

171

Typo -> "Decide file offsets and indexes"

173

For clarity, I'd rephrase this comment as follows:
"We can put section data after the ELF header and the program headers."

177

We need to make sure of what? Perhaps "We need to check both are respected" or something to that effect.

189

Section->Index = Index++; and then drop the later increment.

225

Use typedef.

tools/llvm-objcopy/Object.h

31 ↗

(On Diff #104037)

ParentSegment

tools/llvm-objcopy/llvm-objcopy.cpp

73 ↗

(On Diff #104037)

I think this comment is in the wrong place, or even unnecessary entirely.

tools/llvm-objcopy/llvm-objcopy.h

14 ↗

(On Diff #104037)

Unused header?

[jakehehrlich]

I made the changes recommended by jhenderson. I particularly liked the sorting by offsets recommendation. I also simplified the finalize method in segment. Before it was deciding segment FileSize which it shouldn't and it was using a complicated method to find the minimum when it didn't need to.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

165

This is a really good idea. Thank you!

[jakehehrlich]

I forgot to add all needed full stops in my last update.

[jhenderson]

The implementation looks good from my point of view. I'm wondering about the test coverage. The current test only tests PROGBITS, NULL, and STRTAB sections. However, your original comment comment claims that it will work with NOBITS. I'd like to see that tested, if this is the case. Also test cases with segments in as well, would be good.

tools/llvm-objcopy/Object.cpp

169

"make ensure" -> "make sure" or just "ensure"

[jakehehrlich]

This change adds test coverage for NOBITS and program headers. It turns out there were issues with NOBITS and there were issues with segments. I fixed the bugs exposed as well

Changes from last diff (outside of tests):

1. Some segments (like STACK segments) don't have sections so you can't set their Offset to firstSection()->Offset. This demanded a small change to Segment::finalize
2. Sometimes sections that don't belong in segments would wind up in a PT_LOAD segment that had Offset == 0. This is fixed by only adding allocated sections to segments (see Object::readSectionHeaders)
3. I was unconditionally requesting the contents of a section. This is an error in the NOBITS case so I had to add a check to only request the section contents when there is some. (see Object::readSectionHeaders)

[jhenderson]

In D33964#794100, @jakehehrlich wrote:

2. Sometimes sections that don't belong in segments would wind up in a PT_LOAD segment that had Offset == 0. This is fixed by only adding allocated sections to segments (see Object::readSectionHeaders)

I disagree with this approach - yes non-allocatable sections should not end up in PT_LOAD (although using linker scripts, I wouldn't be surprised if it is possible), but it is perfectly legitimate to have segments that are not loadable with non-SHF_ALLOC sections (see the ongoing discussion in D34204 for explanation). I'm not quite sure what the case here is that you've encountered, but it sounds to me like the issue is elsewhere.

Also, I noticed that you don't ignore SHT_NULL sections, but you probably should. Large proposal 74 ELFs with many sections have metadata stored in that section. I'd suggest two things:

1. Don't add support for Prop74 just yet - you can check somewhere if it is one and then bail out, to be safe.
2. Skip the SHT_NULL section when reading section headers, and recreate it later on.

tools/llvm-objcopy/Object.cpp

107

This should probably be SHT_STRTAB || SHT_NULL.

[jakehehrlich]

I decided to just skip index zero rather than all SHT_NULL sections. It's equivalent for normal files but also handles strange files that want to have SHT_NULL sections in the middle for some reason.

Having the dummy section be treated as a real section was causing the issue I had to fix with the SHF_ALLOC check so fixing this killed two birds with one stone.

[jhenderson]

In D33964#795981, @jakehehrlich wrote:

I decided to just skip index zero rather than all SHT_NULL sections. It's equivalent for normal files but also handles strange files that want to have SHT_NULL sections in the middle for some reason.

Having the dummy section be treated as a real section was causing the issue I had to fix with the SHF_ALLOC check so fixing this killed two birds with one stone.

Great! One minor nit, and I think we're nearly there, from my point of view. Also, at things stand, very large proposal 74 ELFs with metadata in the SHT_NULL section will be corrupted silently, I believe, so it might be nice to error if there are too many sections when trying to write.

tools/llvm-objcopy/Object.cpp

128–129

Sec->Index = Index++?

[jakehehrlich]

Fixed the separate index assignment and increment.

[jhenderson]

tools/llvm-objcopy/Object.cpp

161

address order -> offset order

185

Could you explain why we are using a mix of offsets and addresses here, as it is surprising to me.

196

properlly -> properly

197–198

Why only the sh_name field? 4-byte alignment could cause other struct members to be misaligned (e.g. the sh_addr and sh_offset fields), because they are 8-bytes in size.

[jakehehrlich]

Fixed some small issues like the comments recommend by James.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

185

The first an main point is that the Offsets change in this loop and what we want to respect is the difference between the section offsets in the original file. FirstInSeg->Offset no longer respects that information. This is changeable however by adding fields that keep track of the original file offset. This might make sense to use elsewhere as well like in the sorting algorithm where we're trying to respect the original offset as much as possible. This would free up Offset to be messed with (in what way isn't clear) before sorting occurs. So this is changeable but only by adding another field. Perhaps that is not a bad idea however.

This said when I was thinking about this I never actually even thought to use Offset. I was thinking about respecting the memory image not the file image. I think as a guiding principal "respect the memory image" is a a better guiding principal but only slightly. The memory image and file image should agree for all valid files however so I'm not really opposed to changing it. I have a slight opposition but it's based on some ideas I have for changing the file layout algorithm and I don't want to justify code here based on code that hasn't been written yet. The general idea is that it is possible to enforce "respect the memory image" via const correctness/encapsulation but it is not possible to do the same for the file image because Offset will need to be changed.

My personal opinion is that adding a new Offset like field that isn't quite Offset and using that field to compute the new offset here isn't really any clearer that what I have here. What do you think?

197–198

The next field (sh_type) is also a 4-byte word and thus anything after those two will be 8-byte aligned. So this is the minimal alignment I can choose which ensures that all the fields of the section header are aligned. I updated the comment to reflect this fact.

[jhenderson]

By the way, although it makes it easier for me to see the changes, your current diff only shows the changes versus the previous diff, so has lost, for example, the tests.

As noted in one of my comments, I'm not certain that this will work in the nested segment case. Is there a test for this?

tools/llvm-objcopy/Object.cpp

185

I think I follow what you're saying and broadly agree with it. We have to respect the memory image, at least for linked objects, because if we change the relative positions of two sections within a segment, then the address mapping will no longer be correct. For relocatable objects, that's not an issue, because they don't have a memory mapping yet (and segments can be ignored in this case).

However, the file and memory image are not always consistent, even for valid programs. Specifically, SHT_NOBITS sections have no file image typically, but do have a memory image. Usually, this is not an issue, because the .bss section is placed at the end of the data segment. However, when using TLS (which is sometimes a nested segment in the data segment) this is not the always the case due to .tbss, because other non-TLS data usually appears after it. From my observations, different platforms exhibit different behaviour here, but at least some of them have no file image assigned for the .tbss section, but do have a memory image associated with it. As such, the layout algorithm needs to be able to handle this case.

That all being said, looking at the code without actually trying it out, I'm not convinced that this will handle the nested-segment case anyway, in which case what you've got here is fine. However, I would suggest updating the comment to explain why we need to respect interstitial memory gaps.

197–198

Relative to the start of the section header, the fields will be aligned correctly. However, relative to the start of the file, they won't be: for example, in 64-bit ELF, with 8 byte address fields, the sh_name field was aligned to offset 4, the sh_type field will be at offset 8, and the 8-byte-sized sh_flags field will be at offset 12, which is misaligned.

[jakehehrlich]

1. I was wrong about the alignment of section headers as James pointed out. I fixed that.
2. Added comments recommended by James.
3. This is the full diff that I forgot to add last time

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

197–198

Yep. You're right. I did my math wrong. Not sure what I was thinking there. Thanks for catching that! It should be fixed now.

[jhenderson]

Could you construct a test case that confirms the behaviour of nested segments, please? Something with a memory image like:

| Segment 1                                                        |
| Segment 2                               |
| Progbits Section | gap | NOBITS section | gap | Progbits section |

test/tools/llvm-objcopy/hello-world.s

2 ↗

(On Diff #105513)

I don't think this will work for many people, as LLD is not part of LLVM's main source code body, so people will be unable to run this test. That being said, I'm not sure how to work around this. Can yaml2obj help us here? (I'm not particularly familiar with that tool yet).

tools/llvm-objcopy/Object.cpp

185

I'd suggest changing the second and third sentence to something like:

"We *must* maintain the memory image so that addresses are preserved. As, with the exception of SHT_NOBITS sections at the end of segments, the memory image is a copy of the file image, we preserve the file image as well."

It explains why the file and memory image need to match, which may not be obvious to those who are unfamiliar with ELF.

[jakehehrlich]

In D33964#803460, @jhenderson wrote:

Could you construct a test case that confirms the behaviour of nested segments, please? Something with a memory image like:

| Segment 1                                                        |
| Segment 2                               |
| Progbits Section | gap | NOBITS section | gap | Progbits section |

I can confirm this just won't work right. As is the first Progbits section will wind up in two Segments and everything will get all messed up. For this diff I'm fine with that not being supported. I'm also not sure what the intended behavior is in this case.

test/tools/llvm-objcopy/hello-world.s

2 ↗

(On Diff #105513)

Oh man, that didn't even occur to me. As far as I am aware there is no way to produce program headers using yaml2obj. It never produces any program headers by default either. I think you're right and this should be fixed. I have no clue how to fix it other than adding support for program headers to yaml2obj. That's not something I've looked into however.

Perhaps there is a way to require lld for this test? That way it's just unsupported for most people?

[jakehehrlich]

Fixed the comment as recomended

[jhenderson]

In D33964#803969, @jakehehrlich wrote:

In D33964#803460, @jhenderson wrote:

Could you construct a test case that confirms the behaviour of nested segments, please? Something with a memory image like:

| Segment 1                                                        |
| Segment 2                               |
| Progbits Section | gap | NOBITS section | gap | Progbits section |

I can confirm this just won't work right. As is the first Progbits section will wind up in two Segments and everything will get all messed up. For this diff I'm fine with that not being supported. I'm also not sure what the intended behavior is in this case.

I'm ok with that as well, but I'd like to understand what your plan is going forward to fix this, in case it has a significant impact on the design.

Having a progbits section in multiple segments is absolutely fine, and I'd expect the layout to be maintained as in the diagram. The alignment of all containing segments needs to be respected when placing the first section, and then they all start at the same place (or in the case where the nested segment is not at the start, at the same relative offset). There are some weird oddities when coming to NOBITS sections, because of their file/memory footprint mismatch. The simple, but probably undesirable, option is to allocate a file footprint for any NOBITS sections that are not at the end of the top-most parent segment. I think though that actually, as is the case in the diagram here, that there is special handling for nested NOBITS sections like this in loaders (specifically, I'm thinking of the .tbss case again - the example could easily be a data segment with a nested TLS segment, with the .tdata, .tbss, and .data sections). By my understanding, as long as .tbss is at the end of the nested TLS segment, it should not have a file footprint, even though it makes it look like there'll be a mismatch in the data segment.

test/tools/llvm-objcopy/hello-world.s

2 ↗

(On Diff #105513)

I'm afraid I don't know of a way. There might be, but if there is, somebody else will have to advise.

It seems to me that having program header support in yaml2obj is very desirable. I'd offer to add it, but I do not have any time to do so.

[jakehehrlich]

In D33964#804772, @jhenderson wrote:

I'm ok with that as well, but I'd like to understand what your plan is going forward to fix this, in case it has a significant impact on the design.

Having a progbits section in multiple segments is absolutely fine, and I'd expect the layout to be maintained as in the diagram. The alignment of all containing segments needs to be respected when placing the first section, and then they all start at the same place (or in the case where the nested segment is not at the start, at the same relative offset). There are some weird oddities when coming to NOBITS sections, because of their file/memory footprint mismatch. The simple, but probably undesirable, option is to allocate a file footprint for any NOBITS sections that are not at the end of the top-most parent segment. I think though that actually, as is the case in the diagram here, that there is special handling for nested NOBITS sections like this in loaders (specifically, I'm thinking of the .tbss case again - the example could easily be a data segment with a nested TLS segment, with the .tdata, .tbss, and .data sections). By my understanding, as long as .tbss is at the end of the nested TLS segment, it should not have a file footprint, even though it makes it look like there'll be a mismatch in the data segment.

Well if this is in fact the intended behavior then I think that means I have to switch from "respect the memory image" to "respect the file image". If the relative offsets are used instead of relative addresses it should solve this problem with only a tiny change. If I stick to "respect the memory image" then I'd have to add all sorts of complicated book keeping to ensure that this case was handled correctly. Even if TLS isn't handled this way there might be some case like this where it is and we should respect the choice that the file made. I think this case (or even the possibility of such a case existing) convinces me that it should be "respect the file image" and not "respect the memory image".

So to handle this I propose the following changes:

1. Make the parent segment the lowest offset segment. I think I'd prefer "lowest offset segment" rather than "top segment" in order to account for strange things like overlapping segments in case those are also a thing. I'm slowly coming to realize that it's best to curtail my assumptions as much as possible.
2. Change "Offset = FirstInSeg->Offset + Section->Addr - FirstInSeg->Addr;" to "Offset = FirstInSeg->Offset + Section->OriginalOffset - FirstInSeg->OriginalOffset;" because of the TLS case you found. This way we respect the choice the input file made on this mater regardless of weather it allocates space in the file for the NOBITS section or not.
3. Add a parent segment field to segments so that nested segments (and even overlapping segments) can have offsets based on their parent segment and not the sections they contain.

Does this sound good? If so I'll go ahead and change this diff to use 1) and 2) now and I'll add 3) in another diff. Even with just 1) and 2) I think this should still work in the specific case you requested a test on but I'll wait to add that test when I add 3) I think.

test/tools/llvm-objcopy/hello-world.s

2 ↗

(On Diff #105513)

I'm adding support for program headers to yaml2obj now. Hopefully I can land that change soon.

[jhenderson]

I've spent a fair amount of time today investigating GNU objcopy's behaviour for nested segments. The quick version is that it can get into a slightly broken state if you start messing about with sections in nested segments (although it does the right thing for the normal use cases).

I used an artificial linker script to create an arbitrary nested segment with four non-empty sections in: 3 progbits sections followed by a nobits section. In the parent segment, I also added additional progbits sections and concluded with a nobits section. If objcopy is used to strip one of the progbits sections in the nested segment, the file image is left unchanged, as expected, but perhaps bizarrely, the memory image of the segment gets messed up - the addresses of the sections are unchanged, but the memory image of the segment has unexpectedly shrunk. I suspect this is a bug. The behaviour of the TLS sections gets even weirder, and there appears to be a lot of special case handling which causes things to go wrong when additional segments get stripped, so I highly recommend never adding custom sections to the TLS segment!

Anyway, that all aside, from my investigations and my understanding, what you are proposing seems good. Preserving the relative file offsets will be sufficient to cover all cases that I'm aware of, regardless of how the linker has chosen to allocate file and memory space in the segment for sections.

The overlapping segment case is weird, and I'm not sure how to construct an ELF in that state. What you are proposing sounds reasonable for this as well. The only thing I would say is consider the case where Segment1 overlaps the start of Segment2, which in turn overlaps the start of Segment3. As long as Segment3 gets moved relative to Segment1 (possibly indirectly by moving relative to Segment2), I think we're fine.

[jakehehrlich]

1. Offsets of sections that cover a segment are no longer based on relative addresses but on relative offsets from the original file. This accounts for the normal case and the strange TLS case that James pointed out and it does so regardless of weather the target system makes this choice or not for TLS.
2. If a section covers multiple segments then the parent segment is now canonicalized to be the lowest offset segment

[jhenderson]

tools/llvm-objcopy/Object.cpp

95–101

From a code readability point, it might be worth either breaking this into a small helper function, or to explain the algorithm in comments somewhere.

I'm not asking you to add them at this point, but this is the sort of thing that would be great to have unit-tests around!

193

I've just thought about yet another problem with this:

In some cases, the ELF header and/or Program Header table can appear in a segment before the first section. Consequently, we need to preserve the relative offset to the start of the section, rather than the first segment. At the moment, any space before the first section within the segment is ignored and lost.

This applies to Segment::finalize() too.

219

typo: remaining

261–271

Nothing is using Shdr here. Is this missing a write, or can it be removed?

tools/llvm-objcopy/Object.h

57 ↗

(On Diff #106251)

Two sections with identical addresses will be considered identical by the set, which means that only one will be present. I'm pretty sure overlapping sections are bad, but an empty section could easily share the same start address without any worry. I think you need to compare the pointers themselves to prove they are not identical, i.e. the check becomes:

if (Lhs->Addr == Rhs->Addr)
  return Lhs < Rhs;
return Lhs->Addr < Rhs->Addr;

Optionally, you could test other members as well, e.g. Offset or Size, but ultimately, you're going to need to test that they are not the same section somewhere.

Please also add a test case for this (e.g. two identical empty sections at the same address and offset, or one empty and one non-empty section at the same address and offset).

76 ↗

(On Diff #106251)

if (!Sections.empty()) maybe?

136–141 ↗

(On Diff #106251)

The size of some of these fields is dependent on the Elf Type (64 versus 32). I wonder if these should be based on ELFT? On the other hand, I'm quite happy that we just use the largest size for now.

At the moment, since the template is only instantiated for Elf64LE, this is fine, but it won't work once 32-bit support is added, unless the point where they are assigned to the Elf Header struct is modified to cast the values accordingly.

[jakehehrlich]

1. Changed firstSection to use !empty() instead of implicitly converting size() to a bool
2. Segments should now keep track of empty sections. I don't think this would have caused an issue but I agree that it is a problem none the less. It shouldn't ever affect layout but it could affect other analysis.
3. Added a helper "is covered by" method to clean things up as recommended by James
4. Fixed a spelling mistake pointed out by James

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

193

Yea PT_PHDR is a problem I have run into in extensions of this diff. I think it needs some special handling. I think I can support it when I officially add support for copying data of interstitial gaps and nested segments. It will still require special care however. Would you be ok with me not attempting to handle this right now? At the very least this requires handling nested segments because the PT_PHDR segment must be nested in a PT_LOAD segment.

The needed information is preserved now because the original offset of the segment is stored until the segment offset needs to be computed and the original offset of the section is now stored and kept. So that gap can be recovered when performing layout of the segment. This seems to be enough to confirm that there isn't a fundamental design issue that prevents the PT_PHDR case from being handled.

261–271

This is actually correct I believe. This does point out a lack of clarity in what is going on here however.

As is every section knows where their header belongs so I just pass "Out" to the section and it does the rest. This highlighted code writes the dummy section at the start of the section header table. So there's no missing write or anything. I'll add a comment explaining what's going on here.

tools/llvm-objcopy/Object.h

136–141 ↗

(On Diff #106251)

This was a recommendation that I got from Rui actually. To quote Rui when speaking about LLD.

• Integral types such as Elf{32,64}_{XWord,Word,Addr,Offset} are not useful and better to avoid. We are using uint{8,16,32,64}_t instead. It seems it improves readability. (I honestly don't memorize > the real types of these ELFT types.)
• ELFT::uint, whose size is 32/64 depending on ELF32/64, isn't useful. Always uint64_t to represent a value that can be 32 or 64. The waste of doing this is negligible, but it could drastically simplify > types because if your function uses only ELFT::uint, you can de-template that function by using uint64_t.

I've found this to be true as well. It definitely lets you get rid of a lot of templates. If we just used the templates when you had access to them (Like in Object here) and used the largest type when we didn't (like in Segment or SectionBase) we would get this strange inconsistency in our code. I'd like to avoid that.

[jhenderson]

Thanks for adding empty-section.s. Once you have program header support in yaml2obj, I think it would be good to have the same test for empty sections in program headers, for similar reasons (actually, the way the code was written, I think that test would be the more important one).

tools/llvm-objcopy/Object.cpp

79–80

It's not clear on the surface what is meant by "Covers" (I assume it means within or similar). Also, I think you've got it backwards in the name - it returns true, if the section is in the segment, not the other way around.

Also, I'm not sure what the behaviour of empty sections should be here. My instinct is to say that empty sections are "covered" by a segment, if they appear at the start, or in the middle of a segment, but not if it is only at the end. I could imagine a case with two segments with the first having the same end offset as the start of the next one. If there is an empty section at the meeting point, I don't think it makes sense for it to be in both. From a run-time point of view, I don't think it matters, but it could cause weird looking anomalies in static dumping tools, and also leads to the case of, "if the first segment moves to a lower offset, does the section move with it or stay with the other segment?"

193

I'm not sure it's specific to PT_PHDR - it's possible to put these headers in any arbitrary segment, if you want (though I'm not sure why you would in most cases), e.g. directly in the first PT_LOAD segment, with the appropriate linker script. Also, I don't think it's specific to the headers either - the file image is going to get messed up if there was empty space of any kind not covered by a section before the first section. Rather than preserve the relative offsets of sections to the first section, this code should probably preserve the offset relative to the original segment start.

Example:

| Segment                   |
| gap | Section1 | Section2 |
A     B

The offset of Section1 will be placed at A, if I've followed the code correctly, not at B, where it should be, thus messing up the file and therefore the memory image. Section2 will still be placed immediately after Section1 correctly.

As a side point, this will also have an impact on the alignment of the first section, but assuming the input program is valid, I don't think that this will be an issue.

261–271

Ah, I missed the fact that this was a reference rather than a copy. What you are doing here is fine then. A comment might be helpful though, as you say.

tools/llvm-objcopy/Object.h

136–141 ↗

(On Diff #106251)

Ok, that's fine with me, as long as it was a conscious decision. I could see a case in the future when adding support for things like adding or modifying sections when it may be important to safety check that we haven't overflowed, but that's not relevant at the moment.

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

79–80

I agree, "covers" is not the right word here. I'll say "within" or "inside of" I suppose. I thought I was using "cover" in the colloquial sense it is used when referring to these things but apparently I did not use it correctly. I thought it was an odd way to refer to things. I'll just refrain from using that term.

Oh weather an empty section should belong to a segment I agree and came to the same conclusion by the same reasoning. I plan on never modifying segment size or contents however so I don't think this will cause an issue.

193

The difference between this case and the PHDR case is that it's easy to allocate space for t his gap during layout but it is not easy to retroactively figure out that this segment should be covering the program headers which is space that is not even up for allocation from the perspective of the current algorithm. I'm going to upload a new diff that handles this case but does not handle the PHDR case.

[jakehehrlich]

1. Fixed "sectionCoversSegment" confusion
2. Segments with gaps at the start are now preserved
3. Clarified how the SHT_NULL section was being added

[jhenderson]

One more issue that I've put inline, along with a couple of comment typos, and then there's only the tests to sort out. Good work!

It would be good to get somebody else's eyes on this though, given the scale of the new code.

tools/llvm-objcopy/Object.cpp

91

"belongs" to the second section and -> "belongs" to the second segment and

205

asign -> assign

224

I think this only should happen when the section is not in a segment. Otherwise, the section will move independently of the segment, and thus break our file image preservation. In most cases this is a no-op anyway, because the alignment should already be met, but in the case of slightly dodgy input (misaligned section within a segment), we should probably avoid making things worse by breaking the memory image.

If you do this, the comment at the start of the for loop should be updated.

[jakehehrlich]

fixed comment typos

[jhenderson]

Ok, I'm now completely happy with the code as is (barring one more nit in a comment). Just the testing left.

I don't know how you're getting on with segments and yaml2obj, but there is precedent for checking in pre-built binaries in the Lit tests, if it's not possible to create the inputs from raw assembly or similar. That might be a way forward if yaml2obj support is going to take some time.

tools/llvm-objcopy/Object.cpp

226

Nit: I'd use "if this section is in a segment" instead of belonged in, because something might belong in something else, but for whatever reason wasn't put in there.

[jakehehrlich]

I clarified the comment that James mentioned

I'm hoping to get the yaml2obj program headers change pushed though this week and I'm hoping BigCheese will give this a look. A lot has changed since he last looked at it. I'll also see if Petr can't give this another look. BigCheese should be back from the ISO meeting so hopefully he can give it a look this week. So maybe, just maybe, I'll land this this week or early next week.

[Bigcheese]

Looks good after you fix the use of lld in hello-world.s

[jakehehrlich]

1. Fixed test case issue by removing hello-world.s and adding program-headers.test
2. Removed some unneeded headers

[alekseyshl]

MSan detects use-of-uninitialized-value in these new tests, please fix: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/1855/steps/check-llvm%20msan/logs/stdio

[jakehehrlich]

1. Fixed MSan issue. I was not initlizing all fields of StringNames and the parent segment was sometimes uninitialized. A method "makeSection" was added to handle creation of sections so that it is all done in one place rather than spread out.
2. Hopefully fixed a fatal warning on a mac build be removing a variable deceleration

[jakehehrlich]

Somehow I messed up and uploaded the old hello-world.s test. This fixes that

[jhenderson]

Is there any way yet in yaml2obj for it to add program headers which cover more than just the stated sections (in file and memory terms)? It might be nice to have a test that simulates a segment with nobits sections in, and/or arbitrary gaps before and after sections.

Otherwise the tests look good to me as they currently stand.

[jakehehrlich]

Added MC as a dependency. This resolves the error when building on i686-mingw32-RA-on-linux

[jakehehrlich]

Some builds fail when using "make_unique" because "std::make_unique" is sometimes defined. I fixed it to use llvm::make_uinque everywhere

[jakehehrlich]

There was an issue on windows that caused a "permission denied" error. I *think* this is being caused by tool_output_file also opening a handle to the file and windows not allowing writing to such a file. I removed the tool_output_file code

[jakehehrlich]

Added llvm-objcopy to test/CMakeLists.txt

[jakehehrlich]

1. Switched the ordering on sections in segments to use OriginalOffset instead of Address. This should be the same in general but also handles non-allocated sections being added to non PT_LOAD segments and is consistent with the rest of the code in the system.
2. Added llvm-objcopy to lit.cfg. This caused an odd incompatibility on windows but not linux
3. Added addresses to program-header.test. Without this the tests are invalid and before change 1) this would cause undefined behavior in the ordering of sections in segments because the address was 0x0 for everything. For some reason this error only occurred on windows. It would appear that the allocator on linux increases memory addresses monotonically and this caused the bug to be disguised. So now tests are valid

tools/llvm-objcopy/Object.cpp

 #include "llvm-objcopy.h"
 
 using namespace llvm;
 using namespace object;
 using namespace ELF;
 
 template <class ELFT> void Segment::writeHeader(FileOutputBuffer &Out) const {
   typedef typename ELFT::Ehdr Elf_Ehdr;
   typedef typename ELFT::Phdr Elf_Phdr;
 
   uint8_t *Buf = Out.getBufferStart();
   Buf += sizeof(Elf_Ehdr) + Index * sizeof(Elf_Phdr);
   Elf_Phdr &Phdr = *reinterpret_cast<Elf_Phdr *>(Buf);
   Phdr.p_type = Type;
   Phdr.p_flags = Flags;
   Phdr.p_offset = Offset;
   Phdr.p_vaddr = VAddr;
   Phdr.p_paddr = PAddr;
   Phdr.p_filesz = FileSize;
   Phdr.p_memsz = MemSize;
   Phdr.p_align = Align;
 }
 
 void Segment::finalize() {
   if (firstSection())
     Offset = firstSection()->Offset;
 }
 
 void SectionBase::finalize() {}
 
 template <class ELFT>
 void SectionBase::writeHeader(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart();
   Buf += HeaderOffset;
   typename ELFT::Shdr &Shdr = *reinterpret_cast<typename ELFT::Shdr *>(Buf);
   Shdr.sh_name = NameIndex;
   Shdr.sh_type = Type;
   Shdr.sh_flags = Flags;
   Shdr.sh_addr = Addr;
   Shdr.sh_offset = Offset;
   Shdr.sh_size = Size;
   Shdr.sh_link = Link;
   Shdr.sh_info = Info;
   Shdr.sh_addralign = Align;
   Shdr.sh_entsize = EntrySize;
 }
 
 void Section::writeSection(FileOutputBuffer &Out) const {
-  if(Type == SHT_NOBITS) return;
+  if (Type == SHT_NOBITS)
+    return;
   uint8_t *Buf = Out.getBufferStart() + Offset;
   std::copy(std::begin(Contents), std::end(Contents), Buf);
 }
 
 void StringTableSection::addString(StringRef Name) {
   StrTabBuilder.add(Name);
   Size = StrTabBuilder.getSize();
 }
 
 uint32_t StringTableSection::findIndex(StringRef Name) const {
   return StrTabBuilder.getOffset(Name);
 }
 
 void StringTableSection::finalize() { StrTabBuilder.finalize(); }
 
 void StringTableSection::writeSection(FileOutputBuffer &Out) const {
   StrTabBuilder.write(Out.getBufferStart() + Offset);
 }
 
 template <class ELFT>
 void Object<ELFT>::readProgramHeaders(const ELFFile<ELFT> &ElfFile) {
   uint32_t Index = 0;
   for (const auto &Phdr : unwrapOrError(ElfFile.program_headers())) {
     Segments.emplace_back(make_unique<Segment>());
     Segment &Seg = *Segments.back();
     Seg.Type = Phdr.p_type;
     Seg.Flags = Phdr.p_flags;
     Seg.Offset = Phdr.p_offset;
     Seg.VAddr = Phdr.p_vaddr;
     Seg.PAddr = Phdr.p_paddr;
     Seg.FileSize = Phdr.p_filesz;
     Seg.MemSize = Phdr.p_memsz;
     Seg.Align = Phdr.p_align;
     Seg.Index = Index++;
     for (auto &Section : Sections) {
       if (Seg.Offset <= Section->Offset &&
           Seg.Offset + Seg.FileSize >= Section->Offset + Section->Size) {
         Seg.addSection(&*Section);
         Section->ParentSegment = &Seg;
       }
     }
   }
 }
 
 template <class ELFT>
 void Object<ELFT>::readSectionHeaders(const ELFFile<ELFT> &ElfFile) {
   uint32_t Index = 0;
   for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
-    if (Index == 0) { ++Index; continue; }
+    if (Index == 0) {
+      ++Index;
+      continue;
+    }
     if (Shdr.sh_type == SHT_STRTAB)
       continue;
     ArrayRef<uint8_t> Data;
     if (Shdr.sh_type != SHT_NOBITS)
       Data = unwrapOrError(ElfFile.getSectionContents(&Shdr));
     SecPtr Sec = make_unique<Section>(Data);
     Sec->Name = unwrapOrError(ElfFile.getSectionName(&Shdr));
     Sec->Type = Shdr.sh_type;
     Sec->Flags = Shdr.sh_flags;
     Sec->Addr = Shdr.sh_addr;
     Sec->Offset = Shdr.sh_offset;
     Sec->Size = Shdr.sh_size;
     Sec->Link = Shdr.sh_link;
     Sec->Info = Shdr.sh_info;
     Sec->Align = Shdr.sh_addralign;
     Sec->EntrySize = Shdr.sh_entsize;
-    Sec->Index = Index;
-    Index++;
+    Sec->Index = Index++;
     SectionNames->addString(Sec->Name);
     Sections.push_back(std::move(Sec));
   }
 }
 
 template <class ELFT> size_t Object<ELFT>::totalSize() const {
   // We already have the section header offset so we can calculate the total
   // size by just adding up the size of each section header.
   return SHOffset + Sections.size() * sizeof(Elf_Shdr) + sizeof(Elf_Shdr);
template <class ELFT> Object<ELFT>::Object(const ELFObjectFile<ELFT> &Obj) {
   SectionNames->Name = ".shstrtab";
   SectionNames->addString(SectionNames->Name);
 
   readSectionHeaders(ElfFile);
   readProgramHeaders(ElfFile);
 }
 
 template <class ELFT> void Object<ELFT>::sortSections() {
-  // Put allocated sections in address order. Maintain ordering as closely as
+  // Put all sections in offset order. Maintain the ordering as closely as
   // possible while meeting that demand however.
   auto CompareSections = [](const SecPtr &A, const SecPtr &B) {
     return A->Offset < B->Offset;
   };
   std::stable_sort(std::begin(Sections), std::end(Sections), CompareSections);
 }
 
 template <class ELFT> void Object<ELFT>::assignOffsets() {
   // Decide file offsets and indexes.
   size_t PhdrSize = Segments.size() * sizeof(Elf_Phdr);
   // We can put section data after the ELF header and the program headers.
   uint64_t Offset = sizeof(Elf_Ehdr) + PhdrSize;
   uint64_t Index = 1;
   for (auto &Section : Sections) {
     // The segment can have a different alignment than the section. We need to
     // ensure that both alignments are respected.
     if (Section->ParentSegment) {
       auto FirstInSeg = Section->ParentSegment->firstSection();
       if (FirstInSeg == Section.get()) {
         Offset = alignTo(Offset, Section->ParentSegment->Align);
         Section->Offset = Offset;
       }
       // We should respect interstitial gaps of allocated sections
       Offset = FirstInSeg->Offset + Section->Addr - FirstInSeg->Addr;
     }
     if (Section->Align)
       Offset = alignTo(Offset, Section->Align);
     Section->Offset = Offset;
     Section->Index = Index++;
     if (Section->Type != SHT_NOBITS)
       Offset += Section->Size;
   }
   // 'offset' should now be just after all the section data so we should set the
   // section header table offset to be exactly here. This spot might not be
-  // aligned properlly however so we should align it as needed. This only takes
-  // a little bit of tweaking to ensure that the sh_name is 4 byte aligned.
+  // aligned properly however so we should align it as needed. This only takes
+  // a little bit of tweaking to ensure that the sh_name is 4 byte aligned. By
+  // ensuring that that sh_name is aligned we ensure all fields of the section
+  // header are aligned as well.
   Offset = alignTo(Offset, sizeof(typename ELFT::Word));
   SHOffset = Offset;
 }
 
 template <class ELFT> void Object<ELFT>::finalize() {
   sortSections();
   assignOffsets();
 
   // Finalize SectionNames first so that we can assign name indexes.
   SectionNames->finalize();
   // Finally now that all offsets and indexes have been set we can finalize any
   // reamining issues.
   uint64_t Offset = SHOffset + sizeof(Elf_Shdr);
   for (auto &Section : Sections) {
     Section->HeaderOffset = Offset;
     Offset += sizeof(Elf_Shdr);
     Section->NameIndex = SectionNames->findIndex(Section->Name);
     Section->finalize();
   }
 
   for (auto &Segment : Segments)
     Segment->finalize();
 }
 
 template <class ELFT>
 void Object<ELFT>::writeHeader(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart();
   Elf_Ehdr &Ehdr = *reinterpret_cast<Elf_Ehdr *>(Buf);
   std::copy(Ident, Ident + 16, Ehdr.e_ident);
   Ehdr.e_type = Type;
   Ehdr.e_machine = Machine;
   Ehdr.e_version = Version;
   Ehdr.e_entry = Entry;
   Ehdr.e_phoff = sizeof(Elf_Ehdr);
   Ehdr.e_shoff = SHOffset;

 void Object<ELFT>::writeProgramHeaders(FileOutputBuffer &Out) const {
   for (auto &Phdr : Segments)
     Phdr->template writeHeader<ELFT>(Out);
 }
 
 template <class ELFT>
 void Object<ELFT>::writeSectionHeaders(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart() + SHOffset;
-  Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr*>(Buf);
+  Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(Buf);
   Shdr.sh_name = 0;
   Shdr.sh_type = SHT_NULL;
   Shdr.sh_flags = 0;
   Shdr.sh_addr = 0;
   Shdr.sh_offset = 0;
   Shdr.sh_size = 0;
   Shdr.sh_link = 0;
   Shdr.sh_info = 0;
   Shdr.sh_addralign = 0;
   Shdr.sh_entsize = 0;
 
   for (auto &Section : Sections)
     Section->template writeHeader<ELFT>(Out);
 }
 
 template <class ELFT>
 void Object<ELFT>::writeSectionData(FileOutputBuffer &Out) const {
   for (auto &Section : Sections)
     Section->writeSection(Out);
 }
 
 template <class ELFT> void Object<ELFT>::write(FileOutputBuffer &Out) {
   writeHeader(Out);
   writeProgramHeaders(Out);
   writeSectionData(Out);
   writeSectionHeaders(Out);
 }
 
 template class Object<ELF64LE>;
 template class Object<ELF64BE>;
 template class Object<ELF32LE>;
 template class Object<ELF32BE>;