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"."

LGTM

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.

This LGTM.

Ping

This is tested in D10952, which is a patch that takes advantage of segments with multiple slices and frequently triggers the bug handled here. Without this patch, D10952 will break an existing test.

Sure!

Hi Simon,

No more comments, Meador, LGTM. Thanks for working on this.

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.

Addressed reviewers' concerns.

Committed r231707.

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.

Hi all,

This is the tablegen language, not Python :)

Hi Rui and Shankar,

LGTM

LGTM

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.

Thanks for the suggestions. Here is the new version.

Changed the implementation according to suggestions from Rui and Shankar.

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.

Hi Shankar,

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

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.

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

Hi Davide,

Committed revision 221126.

Thanks Sean and Rui for your code review.

Hi Shankar,
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.

Hi Sean, thanks for your input, I didn't know your write-up about linker scripts, it's great!

Implemented Rui's suggestions.

Hi Rui,

No problem, Rui, I was waiting for your feedback too. Thanks for putting in time to see this.

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.

Hi Shankar,

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.

Thanks Shankar. Committed r219449.

Committed r219353.

Committed r219350.

Committed r219349.

Ping and rebase.

Committed r219334.

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.

I added a test and committed r219034.

Hi Shankar,

Hi Shankar,

Committed r218847.

Makes sense, since soneeded is a stringmap. Thanks! I updated the patch.

Committed r218846.

Committed as r218633.

I have a few minor updates to this patch.

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.