In D67867#1692484, @MaskRay wrote:

My earlier question is about why we need the namespace __llvm_libc at all. From libc/src/string/strcat/strcat_test.cpp I conclude it is for unit testing in an environment that already has a libc (gtest). This should probably be documented.

Can we do things the other way round? No namespace, no __llvm_libc_ prefix. Add the -ffreestanding compiler flag and just define strstr, open, etc in the global namespace. In unit tests, invoke llvm-objcopy --redefine-syms= to rename strstr to __llvm_libc_strstr, and call __llvm_libc_strstr in the tests. For functions that affect global states, gtest will not be suitable. It is good to think how the tests will be organized in the current early stage.

Looks like the author of that patch submitted a new patch to accept alignment of power of twos not 2^<align> after James’ comment :)

In D67656#1680134, @MaskRay wrote:

In D67656#1679119, @jhenderson wrote:

I'm very happy to see this change, but I hesitate to diverge in behaviour from GNU's behaviour. I certainly prefer --set-section-alginment to set it to an arbitrary alignment though, so I think we need to lean on the GNU maintainers to change their end.

The behavior difference is also my concern, so I hold off on this patch. I argue at https://sourceware.org/bugzilla/show_bug.cgi?id=24942#c9 that --set-section-alignment .foo=8 => sh_addralign=256 is counterintuitive. Please chime in if you feel the same.

Addressed review comments.

Changed test comment.

Made e_phentsize 0 when there are no program headers too.

Changed test name

I didn't get to all comments yet, and I also haven't added more tests because currently the object file isn't being create properly. Once I am making a valid ELF file I will do more rigorous testing.

Addressed review comments

Fixed UB bug. Still doesn't write correctly, llvm-readelf says the shstrtab is not null terminated, presumably the other sections are written poorly too.

I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class.

So remove MutableELFObject and put it in ELFObjectFile? This might let ELFObjectFile no longer depend on ELFFile but I'm not entirely sure on that. It's also worth noting that as @rupprecht has pointed out many methods are now linear not constant so without doing some changes (could even be a boolean to track whether any modifications have been made) it probably shouldn't be used yet.

You could also mess about with more objects to hide away the dangerous methods by doing this.

Is this in reference to the earlier point about just modifying ELFObjectFile?

I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all.

I think it isn't that bad to cast this to a base class if we know the methods that will be called in that context will always be safe to call. It is much more controlled this way but still bugs can happen if the base class methods get change. I don't feel too strongly.

One advantage of private inheritance over a private member, I believe, is that you should be able to use the using directive to pull base-class functions into the public interface of the sub-class:

Oh that's very useful I didn't know about that.

Use right diff

Put option to use writer behind flag. Other large changes

Addressed review comments

In D64281#1640799, @jhenderson wrote:

In D64281#1640474, @abrachet wrote:

I've been thinking about this, is size_t safe to use? Some methods from inherited classes use uint64_t on my machine its fine because even unsigned long and unsigned long long have the same width and sign but on a 32 bit machine I think that assigning to a size_t (which is guaranteed to be word size, I think) from a uint64_t might produce a warning. I don't think there are any examples in this patch, but this seemed like the best place to ask.

You should use size_t and consider casting for cases using uint64_t. In a 32-bit build, size_t is usually a 32-bit number (i.e. unsigned int), so theoretically, the compiler could produce warnings for truncation issues when converting from a uint64_t. Since the main thing these indexes are used for is for accessing a vector, the type should match the argument type of std::vector<T>::operator[], which is (usually) a size_t.

In D66063#1639824, @rupprecht wrote:

In D66063#1636993, @jhenderson wrote:

2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.

This approach (not private inheritance, but making ELFObjectFile a private member + exposing specific accessors) seems to be the least fragile approach, but also the most verbose one, e.g. you may have to implement/override all the methods of EF that are used in subclasses.

Use Optional<size_t> to represents parent segments. Addressed other comments.

That should allow you to handle multiple levels of nesting/overlapping etc without too much difficulty.

Hmm if a segment had a list of its child segments, then couldn't layout be done recursively on each child segment? Would this not be more straight forward?

Added parent segment to both section and segment types and methods to find parent segments

I've been thinking about this, is size_t safe to use? Some methods from inherited classes use uint64_t on my machine its fine because even unsigned long and unsigned long long have the same width and sign but on a 32 bit machine I think that assigning to a size_t (which is guaranteed to be word size, I think) from a uint64_t might produce a warning. I don't think there are any examples in this patch, but this seemed like the best place to ask.

Addressed comments

Use the right diff (!)

Addressed comments

In D66063#1636993, @jhenderson wrote:

I've been trying and failing to come up with a good solution to the design. I only see three options:

1. Adding the getHeader function as this patch does and accepting some fragility. In practice, usages within LLVM are probably fine, because we'll have testing, but it might not work as well for those who wish to use the library in their own tools.
2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.
3. Something more radical involving bigger changes in ELFObjectFile to prevent accessing the wrong header within the base class. I don't have anything specific in mind here at the moment.
   
   Does anybody else have any thoughts on this?

Addressed comments

Addressed comments. Made unit tests better.

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Addressed comments

Addressed review comments

Removed superfluous overloads from this patch not later ones

Use size_t for indexes not uint64_t

In D64281#1638215, @rupprecht wrote:

FYI, you'll have to rebase this whole patch series after rL368826 due to Section::getName() changing signature

Addressed review comments

Addressed comments

Ahh sorry about all those unrelated changes! Not sure what happened there probably a bad rebase like you said.

In D66063#1634844, @jhenderson wrote:

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

Change getHeader() to return a reference not pointer.

rebase

Addressed review comments.

Addressed review comments

Fixed failing test for std::distance call.

Made a virtual getHeader method in ELFObjectFile

Addressed review comments

Addressed review comments

Addressed comments

Addressed review comments

Addressed review comments

Addressed review comments

Rebase

rebase

Addressed review comments

Addressed review comments

Moved remove and add out of this patch

Addressed review comments

Addressed review comments

In D65367#1621113, @jhenderson wrote:

I don't think you want to be modifying the number of dynamic symbols at all. That would require you to make other changes to loadable code, which would impact addresses, and effectively require you to completely relink the program.

You may however still need to modify the contents of the existing dynamic symbols (specifically you may need to be able to update their section indexes).

Remove removeDynSymbol from MutableELFObject

Added remove to MutableTable
Added getMutableSymbol to MutableELFObject
Added unit tests for older changes

Added doxygen comments to private interface. NFC