I don't understand the issue here personally. Can you elaborate? The formula also doesn't make sense to me since Sec.Addr isn't really well defined at that point in the code. e.g. this *is* the layout algorithm so you shouldn't be using Sec.Addr here IMO

Yeah I don't suspect anyone will be bitten by this, it speeds up this use case, and speeds up all other use cases, and I don't think there was ever a strong reason for adding this specific behavior.

So confirmed from reading that this behavoir was just copied by a bit of digging in the code itself. I don't believe a real world use case came up that required this. Reading the bug that Fangrui linked but after that this has my LGTM

https://reviews.llvm.org/D41619 is where I think this stuff was introduced for context.

You did it!!!

Nice. This will unblock --only-keep-debug. Thanks Fangrui!

I'd still like to avoid changing the section type for the reason mentioned, at least until the appropriate point in finalization. I don't want this dangling assumption that casts don't always do what you think they should do.

Please wait on another reviewer to give the ok and respond to remaining comments however.

Personally this looks good to me!

I had another thought. Right now the sections first layout algorithm is actually pretty general and doesn't make too much use of being specific to a particular flag. That's a good thing. That bad bit is that we're changing the section type.

My impression is that a large fleet of devices for some reason decided to do this much like the infamous 8-byte aligned note bug. I think we should allow people to cope while giving them the best results possible. I'm personally in favor of adding this for now and removing it later personally.

I guess my question is more, whats the evidence that there isn't a way to avoid the two call overhead? The use case makes sense to me but my impression is that __round_redirector could be declared to have extern "C" linkage and then llvm_libc::round can be an alias for it. That gets rid of 1 layer of indirection. Getting rid of the next call seems more gross to me but I think some additional objcopy magic can be done to make __round_redirector an alias for ::round then I think this would all link correctly, right?

This looks highly promising to me BTW.

I've missed some emails and the round table at llvm. Could you describe more about why this mechanism for redirecting is used? It seems to me that there should be a way for LLVM_LIBC_ENTRYPOINT(round) to be an alias for ::round rather than requiring two (likely tail) calls here. Could you motivate this mechanism?

I'm not sure why I was added but this looks fine to me. If it compiles and runs on everyone's system I can't see how this could be anything but good.

LGTM except a nit

Seems like a good idea to me!

"Interstitial data" is data covered by by a segment but not a section. Gap fill should IMO set the bytes for all of that space. This is consistent with James's recommendation how things work. --pad-to when padding to a page aligned address this can for instance ensure that all executable bytes contain either valid code or a trap instruction.

What's the use case for these two flags? Where has it come up in actual practice that these were needed? I think they basically exist to cover an issue left by the linker that lld no longer leaves. I'm particularly hesitant to add --pad-to

--gap-fill should likely overwrite all interstitial data which includes data at the end of loadable segments not just the data between sections that we previously preserved. I view the --gap-fill use case as being a use case for things like lld's trap filling. Yes this violates the strict interpretation of "don't overwrite segment contents" but its ok in this case I think. Making binaries more secure is a good thing!

To be clear though, we have a test ensuring that -B is ignored, correct?

I'm happy with this since the votes seem to favor this behavior.

LGTM! Thanks for working on this.

What about having ELFcopyConfig inherit from CopyConfig?

We should certainly check perf. I'd also like to wait on this until Roland gets back in order for him to chime in since he had specific examples of use cases where program headers are required as I recall. Specifically the section virtual addresses are not sufficient in the arcane case of prelinking but prelinking isn't really something I know very much about.

If our intent is to prefer using -O over -B when possible (as seems to be the case) then this all looks good to me. I just want an explanation if one exists.

So it seems like we've been preferring the architecture implied by -O instead of the one preferred by -B. My intuition is that it should be the opposite. Did we find a reason to prefer using -O?

The desired behavior is to keep the program headers intact as much as possible. elf utils for instance just flatout ignores them in this case. Program headers are needed by many tools to matchup the virtual addresses with the sections. I think outside of virtual address and memory size there isn't much that's actully needed in practice however. Ideally we wouldn't even change the file size of segments but I think I've structured llvm-objcopy in a way that makes that difficult unfortunately. I'm 100% ok using a distinct layout algorithm for --only-keep-debug as its a special snowflake where the meaning of segments drastically changes and segments are critical to layout. I'm also ok changing the file size of segments as I don't think its likely to cause a problem with tools like debuggers (there also exist other tools that aren't debuggers that need to perform the same kinds of actions).

This is obsolete I think. Clang doesn't use llvm-objcopy for this anymore if I understand correct. @pcc should be able to confirm.

Landing aside, this looks good to me

This looks fine to me

LGTM other than a couple atomicity issues.

This looks awesome!

I think everything but the implementation of genIndex being confusing looks good to me. I haven't actually groked how that code works yet other than the fact that it generates the index tree that I expect and all the surrounding code looks good to me. I understand that some of the trickiness here comes from the fact that you're building from a list of values but trying to generate the tree structure from that list which is hard. I think we can structure that code better; lets see if we can't device a better algorithm.

The code looks fine to me but I don't understand the global details of this. Hopefully Julie can still review things from there but if not we don't get timely reviews then we can still land this.

What's the use case for this and how do we handle return codes when an error occurs but we continue on?

Sorry there's a lot for me to catch up on it would seem. Would it be possible to get an of how having a mutable subclass of ELFObjectFile works us towards our goal of having a library?

In D64790#1588982, @gchatelet wrote:

In D64790#1588231, @jakehehrlich wrote:

First things first, thx a lot for the review. I really appreciate it.

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness.

Ok

I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

Throughout LLVM we mostly use unsigned to represent an alignment. I don't know for sure what was the rationale for using it instead of uint64_t, it can be for historic reasons (just use the machine's word type) or for space reasons.
A quick estimation via git grep -E "^\s+unsigned [Aa]lign.*" | wc -l gives the following number of occurrences (for the whole llvm-project repo)

• unsigned 200
• uint16_t 4
• uint32_t 132
• uint64_t 44

One of the occurrences for the uint16_t is in llvm/include/llvm/CodeGen/TargetLowering.h so for this one at least is seems that storage is important.

This tends to prove that the Alignment type should be parameterized as you suggest.

I'm blindly accepting the CSS but all other code looks fine. I still have that chrome bug where I can't LGTM sometimes but consider this reviewed and accepted by me as well

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness. I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

I think teaching more raw_ostreams about seeking is probably right. I started to propose a solution but it boiled down to make our own abstractions over existing ostreams and what amounts to raw_string_ostream. I think raw_string_ostream and raw_os_ostream should both support seeking

(sorry I just realized this was already landed, I was out when it was bumped).

Sorry looking now.

The code looks good to me. I'll let Julie give the final architectural approval.

As in I'm not quite ready for this to land but It looks preety solid. I think if you respond to those comments I'll take one last look and then give an actual accept

This looks good to me!

I can't verify the windows parts today. If you can wait until tomorrow I can run it on a windows machine to test it.

LGTM

(fixed it)

LGTM (sorry a chrome bug is preventing me from using drop down menus...sigh)

Overall I think this looks good. Others have been following the details far more closely that I've been so I'll defer to them.

Speaking with Roland on this it seems to be that the established protocol for how you match up vaddrs is to use the program headers. Roland invoked arcane scary references about prelinking and other sharp corners about other possible methods but I don't know or understand the details. The guidance I'm getting is that we need to keep the program headers virtual address space correct. He has stated the program headers are used by many existing tools and that the consumers are more varied than just the two major debuggers. If you need more than a "here be dragons" from Roland I'll probe for more information but I'm content enough with just knowing that the existing protocol requires using program headers.

My feeling is that segments are not important. The offsets of those NOBITS sections are not important. If segments are laid out weirdly after objcopy --only-keep-debug, I think it doesn't matter if llvm-objcopy also fails to layout the segments in a sensible way. The important thing here is that debuggers can correctly load debug info from such --only-keep-debug processed files. So I asked you test the workflow:

I think this change as is should not go in. I think I'd like to explain some details that I've learned about this flag and how it functions in practice since I've had to consume binaries produced by this and it's equivalent in eu-strip.

How important is using ErrSaver. Isn't it possible to just use std::string in these error cases?

This is *awesome*. This is much more clear and precise that the GNU objcopy documentation!

Can you upload git diff -U100000 <commit before genHTML> functions so that I can see the code without it being fragmented? It's hard to parse some of the more fragmented stuff here

This all LGTM now. Please wait long enough for others to get their say but overall I'm happy now.

I'm getting confused in all the different patches. I reviewed the HTML generator one like yesterday and was awaiting changes. Doesn't this do the same thing?

Overall looks better. One of my comments causes a sweeping change to occur so I'll respond back after thats done.

How invasive would it be to change this to not use an ostream but to instead use an in memory buffer? It would be useful to know where the use cases come from. Do we intend to use this inside of MC? Do we intend to output raw files? When outputting to MC is the space preallocated? Is there a reason that using an ostream would let us avoid a copy? If we can't avoid a copy or theres not an easy way to know the total size in advance then I'm pro using an std::vector<uint8_t> as the underlying storage mechanism instead of an ostream.

Sorry about the lull here. This LGTM

I got to the 'genHTML' functions and I decided that this general approach is not great.

LGTM

General idea looks good to me!

A bunch of nits but this looks good to me.

Sorry I wasn't very quick to get to this. This looks really good to me. I'm going to go over some of the this with a fine tooth comb tomorrow but It basically looks good as at high level glance.

Addressed Armando's comments

Actually if we can make Description a hash set that would be best. I think using std::set would require an awkward < operator to be defined. There should be standard ways of getting hashes out of all the string and vector types and you can use hash_combine to combine all of these into a single hash.

This is just where I got tired today but I think I can recommend how to split this up so I could move faster and provide more useful high level review. Prior to splitting I'll keep chugging away for at least a bit each day.