ruiu (Rui Ueyama)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 18 2013, 12:16 AM (226 w, 6 d)

Recent Activity

Yesterday

ruiu added a comment to D36351: [lld][ELF] Add profile guided section layout.

I do not actually understand the algorithm yet, but I'm sending out other review comments so that you can fix them early.

Tue, Aug 22, 6:26 PM · lld
ruiu added inline comments to D37044: Fix bug 34051 by handling empty .res files gracefully..
Tue, Aug 22, 6:21 PM
ruiu added a comment to D35387: [MACH-O] Fix the ASM code generated for __stub_helpers section.

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

Tue, Aug 22, 5:10 PM · lld
ruiu accepted D36256: [ELF] Don't output headers into a segment if there's no space for them.

Thank you for adding the comment! I now understand what this function is doing. LGTM

Tue, Aug 22, 4:31 PM · lld
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 4:22 PM
ruiu added inline comments to D33880: COFF: Introduce LD shim around LINK.
Tue, Aug 22, 3:59 PM
ruiu added inline comments to D37031: Handle full path and archive library names in section placement rules.
Tue, Aug 22, 3:28 PM
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 3:27 PM
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 3:06 PM
ruiu committed rL311497: Revert r311468: If --dynamic-list is given, only those symbols are preemptible.
Revert r311468: If --dynamic-list is given, only those symbols are preemptible
Tue, Aug 22, 2:56 PM
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 2:16 PM
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 1:55 PM
ruiu added a comment to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.

Can you upload a patch between SVN head and your patch instead of one between your last patch and your local head? On reviews.llvm.org, I'm seeing the diffs between your patches instead of the main repository and your patch, which is making code review hard.

Tue, Aug 22, 1:35 PM
ruiu added inline comments to D36874: [ELF] - Mention "-z notext" and -fPIC in some error messages.
Tue, Aug 22, 11:49 AM
ruiu added inline comments to D35945: [ELF] - Linkerscript: better diagnostic for INPUT/GROUP commands..
Tue, Aug 22, 11:44 AM
ruiu accepted D36966: [ELF] - Repair dynsym-pie.s testcase..

LGTM

Tue, Aug 22, 11:35 AM
ruiu added inline comments to D36255: Integrate manifest merging library into LLD..
Tue, Aug 22, 11:14 AM
ruiu added inline comments to D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function.
Tue, Aug 22, 11:03 AM
ruiu added inline comments to D36986: [ELF] Handle assignments outside SECTIONS command separately.
Tue, Aug 22, 10:37 AM · lld
ruiu accepted D37015: [ELF] - Do not report multiple errors for single one in ScriptLexer::setError..

LGTM. Thank you for doing this!

Tue, Aug 22, 10:20 AM
ruiu accepted D37009: [ELF] - Fix for "Bug 34238 - LTO is optimizing away symbols referenced from linker scripts".

LGTM

Tue, Aug 22, 10:18 AM
ruiu added inline comments to D36351: [lld][ELF] Add profile guided section layout.
Tue, Aug 22, 10:14 AM · lld
ruiu added inline comments to D37009: [ELF] - Fix for "Bug 34238 - LTO is optimizing away symbols referenced from linker scripts".
Tue, Aug 22, 9:46 AM
ruiu committed rL311468: If --dynamic-list is given, only those symbols are preemptible.
If --dynamic-list is given, only those symbols are preemptible
Tue, Aug 22, 9:33 AM
ruiu closed D36499: If --dynamic-list is given, only those symbols are preemptible by committing rL311468: If --dynamic-list is given, only those symbols are preemptible.
Tue, Aug 22, 9:32 AM
ruiu accepted D36499: If --dynamic-list is given, only those symbols are preemptible.

Since Rafael is on vacation, I'll submit this patch with the following fixes.

Tue, Aug 22, 9:28 AM
ruiu added inline comments to D37013: [ELF] - Add additional information about location when emiting linkerscript's parsing errors..
Tue, Aug 22, 9:13 AM

Sat, Aug 19

ruiu accepted D36820: [Bash-autocompletion] Add support for -std=.

LGTM

Sat, Aug 19, 1:28 PM

Fri, Aug 18

ruiu added a comment to D36256: [ELF] Don't output headers into a segment if there's no space for them.

I'm sorry but I realized that I don't actually understand what LinkerScript::allocateHeaders exactly does, so I don't understand the new code as well. I believe you know better than me, so can you add a function comment to describe what that function is supposed to do?

Fri, Aug 18, 5:29 PM · lld
ruiu committed rL311214: Rename {Lazy,}ObjectKind -> {Lazy,}ObjKind..
Rename {Lazy,}ObjectKind -> {Lazy,}ObjKind.
Fri, Aug 18, 5:15 PM
ruiu accepted D36201: Merge manifest namespaces..

LGTM

Fri, Aug 18, 5:10 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Fri, Aug 18, 4:41 PM
ruiu added a comment to D36874: [ELF] - Mention "-z notext" and -fPIC in some error messages.

I also wonder if you really want to mention -z notext. We want to recommend or endorse the option because we want to avoid text relocations in general. In most cases, you just need to compile object files with -fPIC, so I believe you want to mention only that.

Fri, Aug 18, 4:07 PM
ruiu added inline comments to D36874: [ELF] - Mention "-z notext" and -fPIC in some error messages.
Fri, Aug 18, 4:04 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Fri, Aug 18, 3:05 PM
ruiu accepted D36262: [ELF] - Do not forget to fill last bytes of PT_LOADs with trap instructions..

LGTM

Fri, Aug 18, 2:36 PM
ruiu accepted D36145: [ELF] - Do not segfault when doing logical and/or operations on symbols that have no output sections..

LGTM

Fri, Aug 18, 2:35 PM
ruiu accepted D36607: [Support/Parallel] - Do not use a task group for a very small task..

LGTM

Fri, Aug 18, 2:34 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Fri, Aug 18, 2:18 PM

Thu, Aug 17

ruiu added inline comments to D36820: [Bash-autocompletion] Add support for -std=.
Thu, Aug 17, 9:39 PM
ruiu added inline comments to D36782: [Bash-autocompletion] Add support for static analyzer flags.
Thu, Aug 17, 1:09 PM
ruiu added inline comments to D36820: [Bash-autocompletion] Add support for -std=.
Thu, Aug 17, 1:09 PM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

I *believe* busy-waiting for a very short period of time before putting itself into the blocked state is what the condition variable or the mutex implemented in the standard library already do. If you want to tune parameters for multi-thread programming primitives, you can try, but I personally wouldn't do that because I know it is hard to improve because it's already improved by experts in that field.

Thu, Aug 17, 8:51 AM
ruiu added inline comments to D36820: [Bash-autocompletion] Add support for -std=.
Thu, Aug 17, 8:25 AM
ruiu added inline comments to D36145: [ELF] - Do not segfault when doing logical and/or operations on symbols that have no output sections..
Thu, Aug 17, 8:20 AM
ruiu added inline comments to D36262: [ELF] - Do not forget to fill last bytes of PT_LOADs with trap instructions..
Thu, Aug 17, 7:42 AM

Wed, Aug 16

ruiu accepted D36818: [llvm-dlltool] Improve an error message when unable to open files. NFC..

LGTM

Wed, Aug 16, 11:07 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Wed, Aug 16, 10:04 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Wed, Aug 16, 6:21 PM
ruiu added inline comments to D36255: Integrate manifest merging library into LLD..
Wed, Aug 16, 6:19 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Wed, Aug 16, 5:56 PM
ruiu committed rL311056: Remove a lock and use a std::unique_ptr instead..
Remove a lock and use a std::unique_ptr instead.
Wed, Aug 16, 5:29 PM
ruiu accepted D36138: [ELF] - Don't segfault when accessing location counter inside MEMORY command..

LGTM

Wed, Aug 16, 4:11 PM
ruiu added inline comments to D36573: [ELF] - Treat .gnu.linkonce sections as COMDAT.
Wed, Aug 16, 4:06 PM
ruiu added inline comments to D36262: [ELF] - Do not forget to fill last bytes of PT_LOADs with trap instructions..
Wed, Aug 16, 3:50 PM
ruiu added inline comments to D36145: [ELF] - Do not segfault when doing logical and/or operations on symbols that have no output sections..
Wed, Aug 16, 3:46 PM
ruiu added inline comments to D36255: Integrate manifest merging library into LLD..
Wed, Aug 16, 3:40 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Wed, Aug 16, 3:24 PM
ruiu accepted D36780: [llvm-dlltool] Don't crash if no def file is provided or it can't be opened.

LGTM

Wed, Aug 16, 3:13 PM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

First of all, busy-waiting is a no-no, so we can't eliminate the condition variable from the thread pool. When a thread has to wait for some event to happen, it has to use a condition variable to put itself into the "blocked" state instead of busy-looping until the condition is met. That is in general true even if using busy loops makes your program faster, as your OS supports multiple processes and you don't want one process eat up all CPU resources.

Wed, Aug 16, 2:58 PM
ruiu added a comment to D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB..

Until this patch, lld behavior was deterministic. It generated the same output as long as your input files and command line options are the same.

Wed, Aug 16, 2:23 PM
ruiu added a comment to D36782: [Bash-autocompletion] Add support for static analyzer flags.

This patch allows us to embed a piece of C++ code to each command line option to construct a list of argument candidates at runtime. With this patch, .inc files generated by OptParserEmitter contain C macros that in turn include other .inc files. That is a flexible mechanism but can be too flexible and fragile, as you can write any code in .td file, and pieces of C++ code you write in .td files are not verified even for syntax validation until that nested text inclusions are processed by a C++ compiler. It does two-stages of code generation, one is done by OptTable and the other is done by C macros. I can imagine that a single typo in a .td file can cause a mysterious error that is hard to debug. I feel like, even though command line processing is not that easy, I wonder if it is that complicated to require two-stage code generation.

Wed, Aug 16, 2:23 PM

Tue, Aug 15

ruiu accepted D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB..

LGTM

Tue, Aug 15, 2:18 PM
ruiu accepted D36544: [COFF] Add SymbolName as a distinct field in COFFImportFile.

LGTM

Tue, Aug 15, 1:49 PM
ruiu added inline comments to D36201: Merge manifest namespaces..
Tue, Aug 15, 1:48 PM
ruiu added inline comments to D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB..
Tue, Aug 15, 1:33 PM
ruiu added a comment to D36201: Merge manifest namespaces..

Publishing comments about coding style first. I'll do real code review later.

Tue, Aug 15, 1:27 PM
ruiu added a comment to D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB..

I think it is more straightforward if you change Writer::writeBuildId to compute a hash using not only an executable but also a pdb file. If you do that, every time debug info is different, you'll get a different build id, which I think solves the issue.

Tue, Aug 15, 11:17 AM
ruiu committed rL310934: Fix -Wunused-lambda-capture for Release build..
Fix -Wunused-lambda-capture for Release build.
Tue, Aug 15, 10:41 AM
ruiu committed rL310931: Remove GdbIndexSection::finalizeContents..
Remove GdbIndexSection::finalizeContents.
Tue, Aug 15, 10:02 AM
ruiu committed rL310930: Use ArrayRef instead of std::vector&..
Use ArrayRef instead of std::vector&.
Tue, Aug 15, 10:02 AM
ruiu committed rL310929: Update a comment and rename a function..
Update a comment and rename a function.
Tue, Aug 15, 10:02 AM
ruiu committed rL310925: Remove SymbolTable::findInCurrentDSO..
Remove SymbolTable::findInCurrentDSO.
Tue, Aug 15, 9:04 AM
ruiu accepted D36394: Fix GetEnv to support environment variables with empty values on windows.

LGTM

Tue, Aug 15, 8:34 AM
ruiu added inline comments to D36394: Fix GetEnv to support environment variables with empty values on windows.
Tue, Aug 15, 6:57 AM

Mon, Aug 14

ruiu committed rL310886: Update comments as the function does not write to the first page anymore..
Update comments as the function does not write to the first page anymore.
Mon, Aug 14, 2:19 PM
ruiu added a comment to D36481: /redundancyReport proof of concept.

This is interesting. If we could make the output of this feature usable, and if the feature is useful for other applications than chromium, it might be useful to add this feature to the linker. I'd create a new subdirectory analysis under COFF and put this code there.

Mon, Aug 14, 12:54 PM
ruiu committed rL310864: Add a comment and remove a TODO..
Add a comment and remove a TODO.
Mon, Aug 14, 10:50 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

I still wonder if a communication between threads can make something that slow. Where is the exact location where you observe the slowdown? Until we understand the exact reason, we cannot exclude the possibility that this change is hiding a real issue.

Mon, Aug 14, 9:34 AM
ruiu accepted D36304: [lld] [COFF] Add support for aligncomm directives.

LGTM

Mon, Aug 14, 8:49 AM
ruiu added inline comments to D36394: Fix GetEnv to support environment variables with empty values on windows.
Mon, Aug 14, 8:40 AM

Sat, Aug 12

ruiu accepted D36227: [ELF] - LTO: Try to be option compatible with the gold plugin..

LGTM

Sat, Aug 12, 3:16 PM

Fri, Aug 11

ruiu committed rL310757: Add `-z muldefs` option..
Add `-z muldefs` option.
Fri, Aug 11, 1:50 PM
ruiu added inline comments to D34851: [WebAssembly] Initial wasm linker implementation.
Fri, Aug 11, 10:20 AM
ruiu added inline comments to D36304: [lld] [COFF] Add support for aligncomm directives.
Fri, Aug 11, 9:33 AM
ruiu added a comment to D36567: [Bash-autocompletion] Add --autocomplete flag to 5.0 release notes.

Hans already merged it in r310723.

Fri, Aug 11, 9:26 AM
ruiu added a comment to D36567: [Bash-autocompletion] Add --autocomplete flag to 5.0 release notes.

You seem to have committed a release note for 5.0 to SVN trunk. What you should've done is to do that to the 5.0 branch. I'll correct the error for you.

Fri, Aug 11, 9:21 AM
ruiu added inline comments to D36227: [ELF] - LTO: Try to be option compatible with the gold plugin..
Fri, Aug 11, 9:08 AM
ruiu added inline comments to rL310142: Move File from SymbolBody to Symbol..
Fri, Aug 11, 8:47 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

Well, since it is supposed to be a thread-pool, it shouldn't spawn a thread for a new task. It should keep threads running until a process is terminated.

Fri, Aug 11, 8:26 AM
ruiu added a comment to D36573: [ELF] - Treat .gnu.linkonce sections as COMDAT.

If I understand https://bugs.llvm.org/show_bug.cgi?id=31586 correctly, this is adding more code to a hack that will be removed in near future. I don't think we should do that unless it is absolutely necessary, especially because this patch touches not a single place but many places in the source code.

Fri, Aug 11, 8:25 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

It is broken. The taskgroup should not spawn more threads than the number of physical cores however it is used. It is intended to be a thread-pool, and spawning threads per task is nonsense.

Fri, Aug 11, 8:14 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

Then why does it take so much time in some test cases if both are the same in the big-O? It should never create 65k+ threads. If it does, something's broken. This patch seems to be trying to fix at a wrong place.

Fri, Aug 11, 7:59 AM
ruiu added inline comments to D36227: [ELF] - LTO: Try to be option compatible with the gold plugin..
Fri, Aug 11, 7:44 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

Because the last one is not parallelized by the taskgroup, no?

Fri, Aug 11, 7:37 AM
ruiu added a comment to D36607: [Support/Parallel] - Do not use a task group for a very small task..

This doesn't seem correct. Imagine that you have infinite number of cores. Then, the new code would take 2x time than the old code. I guess you are "fixing" the problem at a wrong place.

Fri, Aug 11, 7:01 AM
ruiu accepted D36466: [ELF] - Do not omit common symbols when -Map is given..

LGTM

Fri, Aug 11, 4:30 AM

Thu, Aug 10

ruiu added inline comments to D36466: [ELF] - Do not omit common symbols when -Map is given..
Thu, Aug 10, 7:11 PM
ruiu added inline comments to D36255: Integrate manifest merging library into LLD..
Thu, Aug 10, 11:21 AM
ruiu added inline comments to D36227: [ELF] - LTO: Try to be option compatible with the gold plugin..
Thu, Aug 10, 11:17 AM