User Details
- User Since
- Apr 18 2013, 12:16 AM (287 w, 1 d)
Today
Please commit.
First round of review.
Yesterday
LGTM
LGTM
First, let me explain my motivation as to why I made changes to the symbol table in the first place, so that you understand the background of my changes.
I'm sorry but we do not guarantee internal API to be the same between
different lld versions. As to removing the utility function, I think
removing a utility function that is used only once in the lld's codebase
makes sense.
Tue, Oct 16
Mon, Oct 15
Honestly I don't think this is worth "fixing" because this is not a bug. We shouldn't bother copying this much details of GNU linkers' behavior. Being said that, since this patch does not append any complexity to the linker, I'm fine with that. LGTM
Sun, Oct 14
LGTM
Fri, Oct 12
Honestly I do not understand the code. What is this code doing? Can you explain it for me?
Can you add a test?
Thu, Oct 11
LGTM
Thank you for doing this!
lld/ELF has Config->Wordsize, so we should add the same variable to COFF.
LGTM
Wed, Oct 10
LGTM
LGTM
Please avoid llvm::formatv because StringRef and Twine work fine as well. I don't expect that the readers of the code memorizes this Python-style format string.
Please submit.
If you are using git monorepo, you can easily combine two or more patches into one on this review site, and when you submit by "git llvm push", the command will automatically make a commit to each backing SVN repository. If you prefer, you can make a separate patch for review though. It's up to you. As long as you get an LGTM, you can commit it to lld.
Tue, Oct 9
It needs to be explained. I couldn't get it just by reading two lines of code.
I don't think I understand the point of this patch. You can't reorder undefined symbols, so why does CG-profile section contains undefined symbol in the first place?
Does that make sense? I still feel like ignoring undefined symbols spoils the point of calling that function in the first place.
You should skip an undefined symbol if it is created from a DSO, no? Looks like just skipping all undefined symbols there also suppresses legitimate warnings.
LGTM
Personally, I found that the way how Go handles pathnames is better than other languages and libraries. In Go, pathname separator is always "/" even on Windows. That means you have to convert from/to native Windows paths by replacing "\" with "/" when you do IO, but having one internal representation makes coding much easier. For example, concatenating a directory name and a filename is as easy as Dir + "/" + Filename; you don't need anything like sys::path::append. Unfortunately, that's not how we handle pathnames in LLVM, though. I'm just saying.
Looking good, but I'd like to wait for pcc's response for a few days if he's okay to have a padding at the end of the section.
If you do this, do you still have to search library files in MinGW driver?
Mon, Oct 8
How did you find it? If you find it in the wild, how large is the padding you added to workaround the issue for your program?
Is this your final patch to support Hexagon, or do you have any other plans? I'm just wondering.
Or, as George pointed out, you can make linker script as independent files.
Hmm, okay, it seems all the choices are not that different, so I'm fine with your original one. Thank you for writing all the pieces of code by the way! :)
I submitted https://reviews.llvm.org/rL343979 which also reduce memory consumption for .zdebug_gnu_pub{names,types} sections, so I don't think we need this anymore.
Fri, Oct 5
LGTM
- make member variables mutable instead of using const_cast to call uncompress()
This function is called only when you did not pass /entry:.
LGTM