LGTM

LGTM

Overall looking good, a few minor comments.

LGTM

LGTM

This is probably OK but I'd like to hear from Rafael as he wrote the code you are updating.

Looks much better. A few minor comments.

I think a better approach would be to change the type of ArchiveName from StringRef to std::string. This patch may fix the issue, but I'm not 100% sure because it fixes only one path that can reach addFile function. There might be some other places at which we pass on-stack objects to the function.

Well, operator<< should still work, but if doing that makes code messy, you don't have to do that.

LGTM

LGTM

It indeed will require extra copy, but that is fine. You don't need to avoid extra copy here since the generated file image is small and this is executed just once.

LGTM

I think a better way of creating manifest file contents is to construct a temporary std::vector and then copy it to a MemoryBuffer after you append everything to the vector. This way, you don't need to compute the size of the result ahead of time.

Yes, overall looking good, but I have one question.

LGTM

LGTM given that you are going to enable GC for other chunks.

I don't have any further comments, but can you please update the patch?

LGTM

Do you have any program that actually needs this change?

I landed https://reviews.llvm.org/rL305920 to add the -exclude-libs flag.

It looks like it's not the only problem for .got but for all sections. How does it work for other sections?

I wouldn't do anything for it unless an evidence that that is actually useful is shown.

LGTM

The rule this patch implements seems too odd that I don't think we should handle this. If you want to sort .ctors or .dtors, I think you can use SORT_BY_NAME. If you use SORT_BY_INIT_PRIORITY on .ctors or .dtors, that is a misuse of the feature.

Can you change your program?

I re-implemented the feature in https://reviews.llvm.org/D34422. The new implementation uses a different strategy than this patch, and by utilizing the relationship of archive files and object files, the new implementation is much simpler and less intrusive than this one.

LGTM. Thank you for fixing this!

If Android needs the flag, I'm happy to re-implement this feature, but can you give me a pointer where the flag is used in Android? I want to understand how the flag is being used before implementing it.

LGTM

LGTM

Hi Andrew,

How about renaming the file MipsArchTree.cpp or something like that?

I think we shouldn't do this. It might make sense to move this file under Arch, but merging it with Arch/Mips.cpp decreases readability as the file now contains totally unrelated functions.

I don't know if we need this, as it implements a feature that is different from what the manual says, and no one is complaining about this.

LGTM

Yes

LGTM

This is beautiful. Overall looking pretty good. Some minor stylistic comments.

LGTM

As to the order of Command variable contents, I think I'm not convinced. You can access the last element as Command[Command.size() - 1] (or maybe Command.back()), no? It is slightly awkward than Command[0], but that's not horribly hard to handle, and passing arguments in the reverse order seems more counter-intuitive to me.

LGTM

LGTM

Doesn't something like work? https://github.com/rui314/llvm-project/commit/5fcc112a76edc12aa58eeb961ba6749525ab11f8

LGTM

LGTM

The first line of a commit message should be shorter than that is now. If you look at other commits, you'll find that their first lines are usually less than 80 or 100 characters.

Rafael, are you ok with this?

LGTM

As a convention, please write a short summary (ideally less than 80 chars) on the first line of a commit message, so that they are not wrapped around
... like this.

OK, that makes sense. But can you simplify the implementation? It seems this patch changes too many places.

If it is not loaded and end of a segment is filled with zeros in memory, it is not a desired result for you, no?

• Rename arch Arch
• Address review comments.

In D33630#781704, @mcgrathr wrote:

In response to ruii's last comment:

I don't think it follows that any *section*'s size should be rounded up. As I just said, that perverts the intent of the link to suggest that the fill bytes are actually part of some section, which they are not.

You may have a case for saying that the *segment*'s size, i.e. p_filesz/p_memsz in the phdr, should be rounded to the page size.
However, given ELF's general statement that up to the whole p_align-aligned area *might* be visible at runtime, I don't really agree with the idea that p_filesz should represent the entire length that we think might be visible.
I think it is better that p_filesz represents the part of the file that *must* be visible at runtime for the program's intent to be correct.
If there were some system or hardware that could choose the accessibility at byte-granularity, then it would be fine and good for it to make only the portion of the file up to p_filesz accessible.

I think I understand the points you made, and I partially agree with that, but we can see it from a different perspective. We fill the gap after executable segments with trap instructions, with the clear intention that the trap instructions will be loaded to memory. So in that sense it makes sense to round up size of executable sections to a page size. Also, in practice, this code seems too large to do a simple thing we've already done for filling gaps between sections. We already have code to fill gaps in LinkerScript.cpp, and this new code seems almost the same.