- User Since
- Aug 13 2014, 4:45 PM (475 w, 5 d)
May 24 2022
I'm logged in another account because phabricator won't let me submit the review I wrote in the original reviewer account. I'm going to paste my full review as a subscriber instead of reviewer to try to bypass the "you can't edit this diff" permission thing. Here it goes:
"treapster changed the edit policy from "All Users" to "Administrators"."
Sep 2 2015
Aug 14 2015
Is it an error in gnu ld to have two output sections with the same name? I'm not sure if it is. IIRC, I remember Rafael specifically relying on the ability to have multiple sections with the same name in order to reduce the final binary size.
Jul 29 2015
Jul 13 2015
Jul 6 2015
Jul 5 2015
Apr 7 2015
Apr 6 2015
Mar 13 2015
No more comments, Meador, LGTM. Thanks for working on this.
Mar 12 2015
Mar 11 2015
Thanks for pointing out Davide, I saw this patch. But there are two things that are not correct in init_array:
Oh you meant, the linking will fail because it is done in the writer (a final step)? No, this is added before the resolver gets called.
For now I just used the same mechanism used to add other linker-defined symbols such as _bss_start, etc. I plan to later change this to be more precise.
Mar 10 2015
Addressed reviewers' concerns.
Mar 9 2015
Thank you all for your patience with this patch size and for the reviews. I'll keep the next patches shorter. Comments inline.
Addressed reviewers' concerns.
Mar 8 2015
Mar 7 2015
This is the tablegen language, not Python :)
Mar 2 2015
Hi Rui and Shankar,
Feb 26 2015
Feb 3 2015
I committed with LLVM_DELETED_FUNCTION because "= delete" breaks my MSVC 2012 box. After we move on to officially only support MSVC >= 2013, we can drop this macro.
Feb 2 2015
Thanks for the suggestions. Here is the new version.
Changed the implementation according to suggestions from Rui and Shankar.
Feb 1 2015
I agree, we don't need a master linker script, Shankar was right in that we
can have multiple linker scripts with the -T option. I'll work on your
suggestions, they sound good to me.
Well, I think no file is left open. After MemoryBuffer::getFileOrSTDIN(), the memory buffer is simply filled in with the file contents, and the file is closed right before this call finishes and the lexer starts its work. So, my original point was that, since we already copied the entire file to a separate memory buffer, we would be duplicating work by creating new strings and duplicating the contents of this memory buffer in several small string buffers.
It's an option, but why dup every string that is already in a memory buffer?
Next time, could you please upload your diffs with "git diff -U999999 <other-branch>"? This way, we get more context here on Phabricator. Thanks for the patch.
I also want to solve the problem of this patch (make ELFLinkingContext own
the buffers) without mixing with future directions on cooking linkerscript
AST. However, we will essentially end up with a vector of BumpPtrAllocators
in ELFLinkingContext (one for each linker script), which makes the code, in
my opinion, much harder to understand (it's better to have "hey, these are
a bunch of linkerscripts" than "hey, here are a bunch of unnamed buffers
that need to be anchored somewhere"). What are the benefits of this? We
avoid including LinkerScript.h into ELFLinkingContext.h, but that is
something that we will eventually need to do to avoid rewriting all classes
in LinkerScript.h in their "cooked" version, don't you think?
The reason I created a new class that encapsulates the parser is because I
plan to later add code to this class (or to another new class called
ScriptSema that is owned by ScriptInstance) that cooks the AST with
semantic info to answer high level questions about the linker script, such
as "in which output section to put this input section, according to this
linker script?". Anyway, I will need to find a location to put new data
structures to store this data, and I thought of this as a first step. What
do you think?
forgot to subscribe to llvm-commits. opened a new one in D7323
Jan 24 2015
Yes, but Clang assumes UTF-8 (Windows or Linux) when no BOM is present. So, even though the system code page is Japanese, when Clang opens input files, it will use a function that interprets the file name strings as UTF8.
Nov 12 2014
Does LLVM generate these "non-ABI-conformant" relocations? Do they need to
be supported by LLD? The relocations are R_X86_64_8, R_X86_64_16,
R_X86_64_PC16 and R_X86_64_PC8.
Another thing, when submitting patches to Phabricator, it is interesting to provide patches with full context (see http://llvm.org/docs/Phabricator.html ). To do this, use
Nov 2 2014
Committed revision 221126.
Oct 27 2014
Thanks Sean and Rui for your code review.
Oct 25 2014
Glad you made a second round of suggestions, thanks. I answered some of your concerns below, but will also work on your suggestions and update the patch soon.
Addressed Sean's concerns and tested the dump'ed scripts. This required adjusting some cosmetic issues to make dump'ed scripts 100% parseable by GNU ld. Also added other missing features to parse other kernel linker scripts: unary negation, fill expressions that can be indefinitely large ( =0x909090909090909090909090) and parsing symbol assignments outside SECTIONS.
Oct 23 2014
Hi Sean, thanks for your input, I didn't know your write-up about linker scripts, it's great!
Implemented Rui's suggestions.
Oct 22 2014
No problem, Rui, I was waiting for your feedback too. Thanks for putting in time to see this.
Oct 21 2014
I changed the AST nodes creation according to Rui's suggestion.
I converted some asserts to error messages, according to Shankar's suggestion. There are still some asserts left that are only to check code sanity and are not appropriate for error messages.
I added test cases that exercises parsing errors (incomplete linker scripts) according to Shankar's suggestion.
I also changed the expression parsing functions to eliminate duplicated code and make it simpler.
Oct 17 2014
I'll address a comment from Rui:
Hi Ruiu, sorry, I messed up this revision because I forgot to add llvm-commits to subscribers. I opened a new one in D5852, I will answer will question there. Thanks for sharing your opinion.
Forgot to add llvm-commits to subscribers. Opened a new revision and added it.
Oct 9 2014
Thanks Shankar. Committed r219449.
Oct 8 2014
Ping and rebase.
You're right, thanks. Changed.
I updated this patch according to suggestions. While this patch doesn't implement the effect of --export-dynamic when building shared libraries (currently, DynamicLibraryWriter already exports all symbols), it doesn't restrict the flag only to executables. I also moved the logic that exports symbols in the dynamic symbol table from OutputELFWriter to the ExecutableWriter class. I think it is not correct to leave this at OutputELFWriter because DynamicLibraryWriter, another subclass of OutputELFWriter, already exports all symbols, meaning we can potentially end up with duplicated symbols in the dynamic symbol table when creating shared libs.
Oct 3 2014
I added a test and committed r219034.
Oct 2 2014
Oct 1 2014
Makes sense, since soneeded is a stringmap. Thanks! I updated the patch.
Sep 29 2014
Committed as r218633.
I have a few minor updates to this patch.
Sep 26 2014
This new version of the patch completely removes the special case for the "tls_get_addr" atom/symbol, since it will be provided by input libraries (usually ld.so). However, some tests depended on tls_get_addr being always defined, independent of inputs. To avoid breaking these tests, this patch adds "--defsym=tls_get_addr=0" to the linker command line in these tests. This patch also updates a "TODO" file that asked to remove tls_get_addr from the output.