Page MenuHomePhabricator

ruiu (Rui Ueyama)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 18 2013, 12:16 AM (295 w, 1 d)

Recent Activity

Today

ruiu accepted D55555: [LLD][ELF][ARM] Add support for architecture v6m thunks.

LGTM

Fri, Dec 14, 4:31 PM
ruiu accepted D55728: ELF: Handle R_ARM_V4BX correctly in PIC output files..

LGTM

Fri, Dec 14, 4:12 PM
ruiu accepted D55443: [test] Capture stderr from 'tar --version' call as well.

LGTM

Fri, Dec 14, 2:38 PM · lld
ruiu committed rLLD349198: Add --plugin-opt=emit-llvm option..
Add --plugin-opt=emit-llvm option.
Fri, Dec 14, 2:02 PM
ruiu committed rL349198: Add --plugin-opt=emit-llvm option..
Add --plugin-opt=emit-llvm option.
Fri, Dec 14, 2:02 PM
ruiu closed D55717: Add --plugin-opt=emit-llvm option..
Fri, Dec 14, 2:02 PM
ruiu added a comment to D55682: [ELF] Support defining __start/__stop symbols as hidden.

Adding a new option is a big deal. Could you elaborate on why you want to make these symbols hidden ones?

Fri, Dec 14, 1:47 PM
ruiu created D55717: Add --plugin-opt=emit-llvm option..
Fri, Dec 14, 1:33 PM

Fri, Dec 7

ruiu added a comment to D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug..

This seems tricky to me. I wonder if you can just fix the problem by reordering symbol assignments in your linker scripts. Also, by allowing forward reference in an assignment, you could theoretically write circular symbol assignments, e.g. foo=bar and bar=foo. That is logically a bit too complicated to me. Maybe we should just keep the linearity of the symbol assignment, instead of supporting this corner case?

Fri, Dec 7, 4:52 PM
ruiu accepted D55218: [ELF] - Allow discarding .dynsym from the linker script..

LGTM

Fri, Dec 7, 4:42 PM
ruiu accepted D55215: [LLD][ELF] - Support discarding .dynstr section..

LGTM

Fri, Dec 7, 4:41 PM
ruiu accepted D55211: [LLD][ELF] - Support discarding the .dynamic section..

This seems like a straightfoward change to allow users to discard .dynamic.

Fri, Dec 7, 2:48 PM
ruiu added inline comments to D55443: [test] Capture stderr from 'tar --version' call as well.
Fri, Dec 7, 12:43 PM · lld
ruiu accepted D55406: clang-format LLVM.h (NFC).

LGTM

Fri, Dec 7, 11:22 AM
ruiu added inline comments to D55443: [test] Capture stderr from 'tar --version' call as well.
Fri, Dec 7, 11:08 AM · lld
ruiu accepted D54875: [WebAssembly] Add support for the event section.

LGTM

Fri, Dec 7, 11:06 AM
ruiu added inline comments to D55443: [test] Capture stderr from 'tar --version' call as well.
Fri, Dec 7, 11:00 AM · lld
ruiu accepted D55441: [lld] [test] Fix reproduce-blackslash.s test with NetBSD tar.

We might be able to change NetBSD's tar but I guess that takes very long time. This test is not very important in the sense that this tests just test a corner case. So I think we should just land this to make it work on NetBSD.

Fri, Dec 7, 10:37 AM · lld
ruiu added a comment to D55441: [lld] [test] Fix reproduce-blackslash.s test with NetBSD tar.

I thought NetBSD's tar is bsdtar because it's BSD... Anyways, I think I'm fine with this change, as the new test (which matches both foo\\.o and foo\.o) does not match a string that we don't want it to match.

Fri, Dec 7, 10:27 AM · lld
ruiu created D55446: Show "Unknown -z option" error message early..
Fri, Dec 7, 10:23 AM

Wed, Dec 5

ruiu added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Sorry, I'm really confused. Could you explain again for me why you need this patch to link Linux kernel?

Wed, Dec 5, 3:27 PM
ruiu added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Looks like what you are trying to do is to generate a x86-64 executable from i386 object files. That's what we do not expect in lld. With this patch, in theory you can create AArch64 exectuables from x86-64 object files, but that doesn't make sense and likely to crash the linker because we assume that all object files are uniform in terms of target types.

Wed, Dec 5, 3:18 PM
ruiu added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Hmm, you said that you are handling inputs that are mix of i386 and x86-64 object files, so I thought this is for that issue. What am I missing?

Wed, Dec 5, 3:10 PM
ruiu added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

Even with this patch, don't you still have to make a change to lld so that it accepts a set of input files of different targets?

Wed, Dec 5, 2:54 PM
ruiu committed rLLD348401: Do not use a hash table to uniquify mergeable strings..
Do not use a hash table to uniquify mergeable strings.
Wed, Dec 5, 11:17 AM
ruiu committed rL348401: Do not use a hash table to uniquify mergeable strings..
Do not use a hash table to uniquify mergeable strings.
Wed, Dec 5, 11:16 AM
ruiu closed D55234: Do not use a hash table to uniquify mergeable strings..
Wed, Dec 5, 11:16 AM
ruiu added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

I ran lld with a few more large programs, and here is the result.

Wed, Dec 5, 11:03 AM
ruiu accepted D45314: [ELF] - (-Map file) Implement printing of LMA for assignments outside of section declarations..

LGTM

Wed, Dec 5, 9:44 AM
ruiu added a comment to D50632: [LLD][ELF] - Simplify TLS LD handling code..

I'd like to see more drastic refactoring of this code. Honestly we should completely rewrite this code because the current code is really hard to read. As to this patch, I'm not sure if this is an improvement from readability perspective, as both the current code and the new code are hard to understand to me.

Wed, Dec 5, 7:54 AM
ruiu added a comment to D55311: Add support for OUTPUT_ARCH linker script command.

What is the motivation to do this? We don't try too hard to implement every detail of the linker script because it's just too complicated and there is usually an easy workaround that works without a linker script.

Wed, Dec 5, 7:36 AM
ruiu accepted D55324: [LLD][ELF] - Linker script: accept using a file name without a list of sections..

LGTM

Wed, Dec 5, 7:34 AM

Tue, Dec 4

ruiu added a comment to D54747: Discard debuginfo for object files empty after GC.

It seems to me that just adding --start-lib to his command line can fix the issue, so I'm waiting for Robert's response. If it doesn't work for some reason, we can analyze why it doesn't work and then discuss what we can do for his problem.

Tue, Dec 4, 4:21 PM · lld
ruiu accepted D55248: [ELF] Simplify getSectionPiece.

LGTM

Tue, Dec 4, 2:26 PM
ruiu added a comment to D55248: [ELF] Simplify getSectionPiece.

Yeah, I think with an optimizing compiler you cannot see any difference between the old and the new code.

Tue, Dec 4, 1:30 PM
ruiu committed rL348294: Remove unreachable code..
Remove unreachable code.
Tue, Dec 4, 11:04 AM
ruiu committed rLLD348294: Remove unreachable code..
Remove unreachable code.
Tue, Dec 4, 11:04 AM
ruiu added inline comments to D54868: [PPC][PPC64] PPC_REL14 and PPC64_REL14 relocations.
Tue, Dec 4, 11:02 AM · lld
ruiu committed rLLD348291: ELF: allow non allocated sections to go into allocated sections.
ELF: allow non allocated sections to go into allocated sections
Tue, Dec 4, 10:52 AM
ruiu committed rL348291: ELF: allow non allocated sections to go into allocated sections.
ELF: allow non allocated sections to go into allocated sections
Tue, Dec 4, 10:52 AM
ruiu closed D55276: ELF: allow non allocated sections to go into allocated sections.
Tue, Dec 4, 10:52 AM
ruiu added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

I will collect more data points even though we cannot share programs with you to be fair, so please wait for a while.

Tue, Dec 4, 10:39 AM
ruiu accepted D55276: ELF: allow non allocated sections to go into allocated sections.

Please add this as a test.

Tue, Dec 4, 10:33 AM
ruiu added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

What we should and do actually care about are programs whose size is multi-gibibyte that take 30 seconds to a few minutes to link. Unfortunately I cannot share the programs with you, but we observed both speedup and memory usage reduction with this patch.

Tue, Dec 4, 10:04 AM
ruiu added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

As one of the people who cares about chromium most in the lld community, I can say that 16 milliseconds regression of linking chromium without debug info is totally negligible.

Tue, Dec 4, 9:49 AM
ruiu added a comment to D55234: Do not use a hash table to uniquify mergeable strings..

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

Tue, Dec 4, 9:10 AM

Mon, Dec 3

ruiu created D55234: Do not use a hash table to uniquify mergeable strings..
Mon, Dec 3, 2:01 PM
ruiu accepted D55231: [WebAssembly] Don't set a maximum side when importing the table.

LGTM

Mon, Dec 3, 1:00 PM
ruiu committed rLLD348153: Show a proper error message if output file is too large..
Show a proper error message if output file is too large.
Mon, Dec 3, 9:46 AM
ruiu committed rL348153: Show a proper error message if output file is too large..
Show a proper error message if output file is too large.
Mon, Dec 3, 9:46 AM
ruiu accepted D55209: [COFF] Don't mark mingw .eh_frame sections writable.

LGTM

Mon, Dec 3, 8:33 AM
ruiu added inline comments to D54868: [PPC][PPC64] PPC_REL14 and PPC64_REL14 relocations.
Mon, Dec 3, 8:31 AM · lld
ruiu added a comment to D55211: [LLD][ELF] - Support discarding the .dynamic section..

Not needed anymore?

Mon, Dec 3, 8:10 AM
ruiu added a comment to D55218: [ELF] - Allow discarding .dynsym from the linker script..

Not needed anymore?

Mon, Dec 3, 8:10 AM
ruiu added a comment to D55215: [LLD][ELF] - Support discarding .dynstr section..

Not needed anymore?

Mon, Dec 3, 8:10 AM

Fri, Nov 30

ruiu accepted D53051: [llvm-tapi] initial commit, supports ELF text stubs.

I do not have any further comments. LGTM

Fri, Nov 30, 3:25 PM
ruiu added a comment to D54747: Discard debuginfo for object files empty after GC.

But 2GB is perhaps still too big and I guess a large part of it can be for dead sections. If we fix this, I'd like to fix it in a proper way so that we can completely eliminate debug info for dead sections.

Fri, Nov 30, 2:04 PM · lld
ruiu committed rLLD348013: Inline a function template that is used only once. NFC..
Inline a function template that is used only once. NFC.
Fri, Nov 30, 10:23 AM
ruiu committed rL348013: Inline a function template that is used only once. NFC..
Inline a function template that is used only once. NFC.
Fri, Nov 30, 10:22 AM
ruiu accepted D55109: [LLD][ELF] - Improve the DWARF v5 suport for building .gdb_index..

LGTM

Fri, Nov 30, 9:13 AM
ruiu committed rLLD348000: Do not assume .idata is zero-initialized..
Do not assume .idata is zero-initialized.
Fri, Nov 30, 8:38 AM
ruiu committed rL348000: Do not assume .idata is zero-initialized..
Do not assume .idata is zero-initialized.
Fri, Nov 30, 8:37 AM
ruiu closed D55098: Do not assume .idata is zero-initialized..
Fri, Nov 30, 8:37 AM

Thu, Nov 29

ruiu created D55098: Do not assume .idata is zero-initialized..
Thu, Nov 29, 5:30 PM
ruiu accepted D55074: [PDB] Quote linker arguments containing spaces (mimic MSVC).

LGTM

Thu, Nov 29, 2:15 PM
ruiu added a comment to D55074: [PDB] Quote linker arguments containing spaces (mimic MSVC).

Looks good, but I just want to make sure that you have already run the exact same test with MSVC to get the same result.

Thu, Nov 29, 2:00 PM
ruiu accepted D55043: [WebAssembly] Allow undefined symbols when building shared libraries.

LGTM

Thu, Nov 29, 11:30 AM
ruiu added inline comments to D50632: [LLD][ELF] - Simplify TLS LD handling code..
Thu, Nov 29, 11:16 AM
ruiu added a comment to D54621: [ELF] - Do not remove empty sections referenced in LOADADDR/ADDR commands..

I really don't like to add more code to the linker script support to make it more and more compatible with GNU, as there is not official language spec and there will always be an incompatibility. How much important is this patch? I think you should discuss that more before start trying to fix. We should not try to "fix" all the reported "issues".

Thu, Nov 29, 11:12 AM
ruiu accepted D55029: set default max-page-size to 4KB in lld for Android Aarch64.

LGTM. Please commit.

Thu, Nov 29, 10:51 AM

Wed, Nov 28

ruiu added inline comments to D53051: [llvm-tapi] initial commit, supports ELF text stubs.
Wed, Nov 28, 6:28 PM
ruiu accepted D54982: [WebAssembly] Update docs.

LGTM

Wed, Nov 28, 4:26 PM
ruiu added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

Yeah, I believe this patch is fine to submit, but since Peter is in a different timezone, I wanted give him a chance to review this one.

Wed, Nov 28, 3:13 PM
ruiu added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

I wouldn't rush to submit this now, given that this issue is not new at all. Maybe we can just wait for Peter's response?

Wed, Nov 28, 2:59 PM
ruiu added a comment to D54747: Discard debuginfo for object files empty after GC.

Thank you for the patch.

Wed, Nov 28, 2:57 PM · lld
ruiu accepted D54483: [ELF] .gdb_index: use lower_bound to compute relative CU index in an object file.

LGTM

Wed, Nov 28, 2:48 PM
ruiu added a comment to D55029: set default max-page-size to 4KB in lld for Android Aarch64.

Somewhat tangent to this patch, but is 64KiB a reasonable default for -z max-page-size? I believe that max-page-size is set to 64KiB so that OS/CPU whose minimum page size is 64KiB can load an executable linked with lld, but is there any real target OS/CPU that does not allow 4KiB pages? Or, is there any other reason to use that default value?

Wed, Nov 28, 2:35 PM
ruiu added a reviewer for D55029: set default max-page-size to 4KB in lld for Android Aarch64: peter.smith.
Wed, Nov 28, 2:33 PM
ruiu added a comment to D54875: [WebAssembly] Add support for the event section.

Overall, this patch needs more comments. lld should be readable to those who knows what the linker is but don't know too much about wasm, so you should explain what event symbol/section/etc. are.

Wed, Nov 28, 1:22 PM
ruiu added inline comments to D54982: [WebAssembly] Update docs.
Wed, Nov 28, 11:30 AM
ruiu added a comment to D54985: [ELF] Keep empty In.RelaIplt so that __rela_iplt_{start,end} have valid st_shndx.

This works, but it may better use a SHF_ALLOC section (not sure if that is what ld.bfd uses)

Wed, Nov 28, 11:24 AM
ruiu added inline comments to D53051: [llvm-tapi] initial commit, supports ELF text stubs.
Wed, Nov 28, 11:17 AM
ruiu committed rL347781: Simplify Symbol::getPltVA..
Simplify Symbol::getPltVA.
Wed, Nov 28, 9:46 AM
ruiu committed rLLD347781: Simplify Symbol::getPltVA..
Simplify Symbol::getPltVA.
Wed, Nov 28, 9:46 AM
ruiu closed D54981: Simplify Symbol::getPltVA..
Wed, Nov 28, 9:46 AM
ruiu added a comment to D54985: [ELF] Keep empty In.RelaIplt so that __rela_iplt_{start,end} have valid st_shndx.

It feels this patch modifies too many places to fix a cosmetic issue. Doesn't something like this work? https://gist.github.com/rui314/6382995c81689668961caf49a0e695f9

Wed, Nov 28, 9:26 AM
ruiu accepted D54624: [LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects.

LGTM

Wed, Nov 28, 8:27 AM

Tue, Nov 27

ruiu added a comment to D54968: [ELF] --gdb-index: use const char *, uint32_t to replace CachedHashStringRef.

Thank you for measuring it. Given these numbers, I wouldn't land this change as StringRef is more straightforward.

Tue, Nov 27, 3:53 PM
ruiu created D54981: Simplify Symbol::getPltVA..
Tue, Nov 27, 3:31 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

I see your point, and I agree with that. But the new code is also error-prone because you can make a mistake to not to add the header size, for example. So not sure which is better.

Tue, Nov 27, 3:29 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

Hm, the new code doesn't look great as it contains too many repetitions. Do you have any suggestion to make this code better?

Tue, Nov 27, 3:13 PM
ruiu added a comment to D54968: [ELF] --gdb-index: use const char *, uint32_t to replace CachedHashStringRef.

I tried this patch myself, but it is hard to say if this patch makes a difference. Probably the performance difference is negligible.

Tue, Nov 27, 3:00 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

Yeah, but the reason why this was just a sketch is because I believe TargetInfo::getPltEntryOffset no longer make much sense with this patch. Perhaps that should be removed.

Tue, Nov 27, 2:42 PM
ruiu added a comment to D54651: [ELF] Allow --noinhibit-exec to produce corrupted executable with relocation overflow.

In order to make lld's --noinhibit-exec behave like GNU linkers', we need to classify errors into "hard errors" (when that happens we need to stop at a nearest checkpoint) or "soft errors" (when that happens we can continue but the output is not likely to make sense.) We cannot simply continue executing the linker after all error()s until the linker creates an output file because some errors are not continuable, and if we force linker to continue, the linker may crash because of an internal inconsistency. I'm fairly satisfied with the current lld's error handling, so I personally don't think categorizing errors into two groups is worth doing though.

Tue, Nov 27, 2:29 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

This is a rough sketch but I believe this is towards a right direction: https://gist.github.com/rui314/d919496a6db34483d673f9ebe34d6f89

Tue, Nov 27, 1:40 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

Then maybe the function is currently at a wrong place?

Tue, Nov 27, 1:09 PM
ruiu added a comment to D54968: [ELF] --gdb-index: use const char *, uint32_t to replace CachedHashStringRef.

I believe the memory savings of this patch is not free; it now has to scan the same string more than once to find a null terminator. Even though we want to reduce memory usage of lld for Google, in general, saving memory is not always the goal that we want to achieve. We need to get a right balance. To do that, can you tell me how much memory you can save with this patch, and how much is the performance penalty of this patch?

Tue, Nov 27, 1:07 PM
ruiu added a comment to D54917: [ELF] Delete TargetInfo::getPltEntryOffset and simplify Symbol::getPltVA.

Yes, but you can teach which is the case.

Tue, Nov 27, 1:02 PM
ruiu accepted D54495: [LLD] [COFF] Remove empty sections before calculating the size of section headers.

LGTM

Tue, Nov 27, 11:34 AM
ruiu added a comment to D54946: [yaml2obj] [COFF] Subtract the image base for section virtual addresses.

Oh, I misunderstood this patch. Sorry for the confusion. My preference was just what I wrote in the previous comment, but that's not a strong preference or anything, and it also makes sense to make values in a YAML file the same as the ones in the actual file, so this patch is fine to me.

Tue, Nov 27, 10:37 AM