LGTM. Would it be appropriate for this patch to add non platform specific implementations that always fail and set errno to ENOSYS in libc/src/sys/mman?

Ping :)
Do you need someone to push this for you?

I just checked quickly and it looks like gtest actually prints to stdout which surprised me, I always figured it was to stderr. Do you think we should go back to stdout?

This is a very big patch and I don't have too much time today so I haven't reviewed this all and I focused on llvm style for now. A lot of comments I only made once but apply throughout.

This patch is fine but my build fails (unrelated to this patch)

make[3]: *** No rule to make target 'projects/libc/src/errno/CMakeFiles/__errno_location_objects.dir/errno_location.cpp.o', needed by 'projects/libc/src/errno/__errno_location_raw.o'.  Stop.

But this happened before applying this patch (and after too). On CMake 3.7 and 3.16. Does it build for you?

In D72102#1806753, @sivachandra wrote:

In D72102#1806672, @abrachet wrote:

Shouldn't these all be static as well, actually?

We really want these functions to be inlined. So, I am not sure how static helps in such scenarios. If they are not inlined, there are probably some advantages in ELF land, but we end up with multiple copies.

Shouldn't these all be static as well, actually?

Generally speaking this kind of change can be pushed without review.

LGTM. The biggest thing at this stage is if our current implementation will make it such that it is too difficult to change later in the pursuit of supporting more architectures and I don't think that is the case here. Even if our internal handling of PAGE_SIZE will change later I think this revision is fine as it is. Wait for someone else to accept too of course.

In D71094#1775691, @MaskRay wrote:

@abrachet It seems you have more comments that cannot be simply categorized as minor updates after you accepted the patch (within 40 minutes.after the patch was posted) Phabricator just simply marks the patch "green" unless another person marks it "Request Changes". There are some controversial parts that may give rise to public disagreement. For such cases, it might be better waiting a bit. (For this patch, I feel that if I did not click "Request Changes" in time, it would be committed and it would be more difficult to improve the situation later.)

I think this is in a good spot, it's easy to read and modify later as needs change. I don't know much about TableGen, I've tried my best to look through, but it would be preferable if other reviewers would be able to take a look too. Up to your discretion how long you would like to wait to let others chime in. LGTM

In D70197#1752985, @sivachandra wrote:

In D70197#1752533, @abrachet wrote:

Not of huge importance, more of curiosity, but could you do what you did with a past revision where you pushed a review with an svg file to your fork so we could see what it looks like on your GitHub?

Uploaded here: https://github.com/sivachandra/llvm-libc/blob/master/docs/header_gen_scheme.svg

Not of huge importance, more of curiosity, but could you do what you did with a past revision where you pushed a review with an svg file to your fork so we could see what it looks like on your GitHub?

LGTM I see no problems from what is here. I'd also like to say that the emphasis on writing docs for all of this is something I'm really happy to see.
I look forward to seeing how the generator works. I am also cautiously optimistic about how well this mechanism will work to create more exotic functions and typedefs. signal(3) and jmp_buf will surely be interesting interesting :)

Seems like a hack, no? What about using --keep-section when you need it? You say its not strictly needed on modern systems and the problem is with the dlopen implementation of these older libcs, and not llvm-objcopy, so I would be hesitant to add something like this especially given we have a solution with --keep-section

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