- User Since
- Apr 18 2013, 12:16 AM (218 w, 3 d)
Fri, Jun 23
Overall looking good, a few minor comments.
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.
Thu, Jun 22
Well, operator<< should still work, but if doing that makes code messy, you don't have to do that.
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.
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.
Wed, Jun 21
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?
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.
Tue, Jun 20
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.
Mon, Jun 19
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.
Sun, Jun 18
This is beautiful. Overall looking pretty good. Some minor stylistic comments.
Sat, Jun 17
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, but that's not horribly hard to handle, and passing arguments in the reverse order seems more counter-intuitive to me.
Fri, Jun 16
Doesn't something like work? https://github.com/rui314/llvm-project/commit/5fcc112a76edc12aa58eeb961ba6749525ab11f8
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?
Thu, Jun 15
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.
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.