- User Since
- Apr 18 2013, 12:16 AM (261 w, 3 d)
Fri, Apr 20
Don't you have to change the test?
- add more flags
- add tests
No I don't. LGTM
LGTM with these fixes.
Thu, Apr 19
If that's the case, --start-lib implies --start-group. To handle it, I'd set InputFile::IsInGroup to true for --start-lib just like we are doing for --start-group, so that --start-lib is handled as if --start-group everywhere.
Does --start-lib/--end-lib really implies --start-group/--end-group?
I wonder if you really need to merge .toc into .got. If I understand your description correctly, it doesn't matter whether they are merged or not as long as they are close enough. So I'm wondering if you can just manipulate the section output order so that .toc and .got are output next to each other.
Please add a test for the linker script.
I think you need to fix readGroup in ScriptParser.cpp as well.
What do you mean by "as if they were listed in --symbol-ordering-file"?
And this should also slightly improve performance because of less number of page faults and improved locality for hot paths, I guess?
This is not actually that important, but I think I prefer my original suggestion. At least we shouldn't use assert() to report a broken file.
Yeah, this seems like a good fit, as long as there is no objection to renaming GotPltSection and PltSection for EM_PPC64.
Wed, Apr 18
I believe a fast strlen() can be implemented using SSE instructions. And if you are using SSE instructions, your data is loaded to XMM registers. I believe there exists a fast vectorized hash function that works on data on a XMM register. I wonder if we can combine the two to create a single fast function that returns the length of a string as well as its hash value.
Tue, Apr 17
- address a review comment
Mon, Apr 16
Fri, Apr 13
Thu, Apr 12
- fix unsafe memory access issue
- address review comment
- convert more tools to use InitLLVM
- set banners to ExitOnError objects
GetArgumentVector is used from a variety of places not all of which share the same code exit behavior (though most do). The same is true for PrintStackTraceOnErrorSignal and PrettyStackTraceProgram. Since all of these are frequently used together (and it's probably an omission of they are not), I agree it would be a great idea to create a simpler pattern than can be used by tools. To properly make the change, we would have to update all the tools to use the new pattern, which is also out of our scope right now. Perhaps we should open a bug/task to be addressed later?
I found that this patch isn't correct. Sorry guys. The issue is that we need to reverse the contents after applying relocations. This patch reverses it before applying relocations. I need to fix that. I'll update the patch.
I don't think I understand what you are trying to achieve with this patch in the first place, as you didn't explain it anywhere. Can you write a patch description properly so that we can understand the intention of the change and what the change is?
This is a mechanical change to replace std::sort with llvm::sort.
What is your operating system and CPU?
I think I'm convinced. We probably should enable that particular test if Python 3.
So, I mean I think what this patch is correct, but orthogonal to that, we should disable the failing test on Windows so that we don't think too hard about character conversion round trip issues.
Thank you for the detailed explanation! That's very helpful. I agree that we shouldn't make a guess on character encoding of command line arguments, instead, we should use Windows APIs to get them in the correct encoding and then convert them to UTF-8. That definitely looks like the right direction.
Wed, Apr 11
What are you actually trying to fix with the patch? Did you just find that lld/test/ELF/format-binary-non-ascii.s didn't pass on your machine and trying to fix it?
That website recommends we handle all strings as UTF-8 whether externally or internally, and if you need to handle strings encoded not in UTF-8, you convert them to UTF-8 first. I'd agree that principle.
lld/test/ELF/format-binary-non-ascii.s may not run properly on Windows if the expected encoding for the command line option is not UTF-8, as the test file contains a pathname containing a non-ASCII character encoded in UTF-8 encoding. That is not immediately an issue on Windows.
Would never having a null InputFile be sufficient?
The reality is that we already have Synthetic files, we just have a very special representation for them: a null pointer.
Since the order of two files is arbitrary, how about just giving each InputFile a number when it is created ?
I'm still in doubt if this is overall a win. At least this patch itself doesn't demonstrate the benefit of having an extra type of file, and I doubt if it will be. Can this simplify things significantly? I think I'm not convinced that we want this one.
Isn't it more complicated than before? I understand that it is tempting, but generally speaking, I try to *not* abstract things unless doing it is a clear win. I'm a bit worried the direction of your recent "refactoring" patches as it adds more abstractions for a (in my opinion) marginal benefits. Factoring out common code is not always a win in terms of readability, especially when it involves callback functions and such. Simple, repeated code is sometimes better, and the original code was written with that in mind. I really prefer boring code over clever code that uses higher order functions.
Yeah, but the it looks up the hash table N times where N is the number of local symbols, no?
George, can you try to not use hash table like this in the future? Both this patch and your previous patch use a hash table to store a lot of symbols, which is a violation of the rule I set to achieve a good performance. If it is unavoidable, please explain in the commit message so that we can discuss and examine if it is really unavoidable. Just using a hash table without an explanation isn't good.
I mean just sorting a symbol shouldn't take 0.73% of the total execution time.
Strictly speaking, this is a violation of the rule that we shouldn't use a hash table for symbols other than the Symbol Table. Is there any way to avoid using a new hash table?
- fix a test and confirmed that the test fails without this patch
- replace dangling shared symbols even if --gc-sections is not specified
I"m not sure if I understand https://bugs.llvm.org/show_bug.cgi?id=28335 correctly, but it sounds like it fixes that bug as well. The intention of this change is, when a DSO is eliminated because of the garbage collection, all traces of the DSO should be removed as if the DSO weren't present in the command line from the beginning.
- address review comments
This change was created by mistake.
- address review comments