Page MenuHomePhabricator

ruiu (Rui Ueyama)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 18 2013, 12:16 AM (330 w, 5 d)

Recent Activity

Today

ruiu accepted D66466: [LLD] [COFF] Print the file name on errors writing the pdb file.

LGTM

Tue, Aug 20, 4:23 AM · Restricted Project
ruiu added a comment to D63109: lld-link: Reject more than one resource .obj file.

We are using FileOutputBuffer. That class creates a temporary file then atomically rename it a desitnation filename when commit() is called. So I'd think we should add a checkpoint just before commit().

Tue, Aug 20, 4:22 AM · Restricted Project
ruiu added a comment to D66426: [lld] Enable a watermark of loadable sections to be generated and placed in a note section.

IIRC lld's --build-id={md5,sha1} was slow at first but after we made them a tree-hash to utilize mutli-cores, its cost became negligible. Have you considered taking that approach? This watermark hash is probably not a thing that people attack by the collision attack, so it might not have to be a cryptographically-safe hash, but still cryptographically-safe hash function has nice properties compared to non-crypto ones.

Tue, Aug 20, 2:57 AM · Restricted Project
ruiu accepted D65242: [ELF] More dynamic relocation packing.

LGTM

Tue, Aug 20, 1:42 AM · Restricted Project
ruiu accepted D65865: [ELF][X86] Allow PT_LOAD to have overlapping p_offset ranges on EM_386.

LGTM

Tue, Aug 20, 12:54 AM · Restricted Project
ruiu accepted D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges.

LGTM

Tue, Aug 20, 12:53 AM · Restricted Project
ruiu accepted D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

LGTM

Tue, Aug 20, 12:53 AM · Restricted Project
ruiu added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

I'm personally in favour of accepting D64930 and D64906 (I think that there is also D65865 on X86) as I think that this is an important size optimization already performed by Gold and BFD. Thinking about how to progress this further, I think it would be good to write a summary of where each of these reviews is at and what the plan is for this feature. For example:

Thanks for pushing forward this patch series!

  • Have all the comments been addressed, I think the main concern was about TLS?

All addressed. There was concern about TLS but I aligned p_vaddr/p_offset to p_align of PT_TLS.

  • Which targets do we want this to apply to, all of them, or just those with a 64k page size?

All, probably except x86_64 (with max-page-size=4096).

bool enable = config->emachine != EM_X86_64 || config->maxPageSize != 4096;

  • What is the dependency tree of the reviews, I think D64906 is a pre-requisite for all of them?

A tree with a single internal node:) Yes, D64906 is a pre-requisite for all of them.

D64906 base, PPC64
  D64930 AArch64 47 tests
  D65865 i386 33 tests
  Dxxxxx ARM (30~50 tests)
  Dxxxxx MIPS
  rLLDxxxxx RISC-V <10 tests
  rLLD PPC32 <10 tests
  Dxxxxx x86_64 with maxpagesize != 4096

Each new target can be enabled independently. The code change is trivial:

-      bool enable = config->emachine == EM_PPC64;
+      bool enable =
+          config->emachine == EM_AARCH64 || config->emachine == EM_PPC64;

The only problem is the number of the affected tests. Due to the layout change nature of the patch series, lots of addresses have to be updated. x86_64 with maxpagesize=4096 is estimated to affect 300+ tests.

  • Does anyone have any objection, or are they happy for individual people active in a backend to approve that part?

We haven't heard from MIPS yet. @atanasyan

@ruiu Let's move D64906, D64930 and D65865 forward?

Tue, Aug 20, 12:34 AM · Restricted Project

Yesterday

ruiu accepted D65903: [LLD] [COFF] Allow using custom .edata from input object files.

LGTM

Mon, Aug 19, 10:47 PM · Restricted Project
ruiu added inline comments to D65903: [LLD] [COFF] Allow using custom .edata from input object files.
Mon, Aug 19, 10:38 PM · Restricted Project
ruiu accepted D66435: [WebAssembly][lld] Fix crash when applying relocations to debug sections.

LGTM

Mon, Aug 19, 9:53 PM · Restricted Project
ruiu added inline comments to D66239: [LLD][COFF] Add index to disambiguate archive members when using -wholearchive .
Mon, Aug 19, 4:39 AM · Restricted Project, lld
ruiu added a comment to D66130: [ELF] Initialize 2 fields of Symbol in SymbolTable::insert.

It might make sense to zero-initialize a new instance with memset(sym, 0, sizeof(Symbol)) before setting any (non-zero/false) values to sym members, as it is less error-prone. What do you think?

Mon, Aug 19, 4:39 AM · Restricted Project
ruiu added a comment to D66275: [ELF][Hexagon] Improve error message for unknown relocations.

LGTM but please wait for Sid.

Mon, Aug 19, 2:57 AM · Restricted Project
ruiu added a comment to D39324: [lld] Support TLS in RISC-V.

This has been merged on July 2 as http://reviews.llvm.org/rL364813

Mon, Aug 19, 2:48 AM · lld
ruiu added a comment to D66274: [ELF][Hexagon] Replace R_HEXAGON_GOT with R_GOTPLT.

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

Mon, Aug 19, 2:39 AM · Restricted Project
ruiu added a comment to D66306: Fix lld on GCC 5.1 after the C++14 move.

LGTM

Mon, Aug 19, 1:44 AM · Restricted Project
ruiu added a comment to D66105: Support HEX_32 when building shared objects.

LGTM

Mon, Aug 19, 1:06 AM · Restricted Project, lld
ruiu added inline comments to D66279: [ELF] Make LinkerScript::assignAddresses iterative.
Mon, Aug 19, 1:06 AM · Restricted Project
ruiu added a comment to D66274: [ELF][Hexagon] Replace R_HEXAGON_GOT with R_GOTPLT.

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

Mon, Aug 19, 12:58 AM · Restricted Project
ruiu added a comment to D66274: [ELF][Hexagon] Replace R_HEXAGON_GOT with R_GOTPLT.

Nice! Thank you for doing this.

Mon, Aug 19, 12:03 AM · Restricted Project
ruiu accepted D66359: [lld][WebAssembly] Honor --no-export-dynamic even with -shared.

LGTM

Mon, Aug 19, 12:00 AM · Restricted Project

Sun, Aug 18

ruiu accepted D66367: [LLD] [COFF] Require an explicit -implib option for creating implibs in mingw mode.

LGTM

Sun, Aug 18, 11:48 PM · Restricted Project
ruiu accepted D66260: [lld][Hexagon] Add GOTREL relocations.

LGTM

Sun, Aug 18, 11:43 PM · Restricted Project, lld
ruiu accepted D65964: [lld-link] implement -lto-obj-path.

LGTM

Sun, Aug 18, 11:13 PM · Restricted Project

Fri, Aug 9

ruiu accepted D65875: [ELF] For VS-style diagnostics, prefer using the full path..

I confirmed that these tests are not affected by the actual path on a machine running the tests. I missed the part that you added .line directive in this patch. Thank you for doing this.

Fri, Aug 9, 12:25 AM · Restricted Project, lld

Thu, Aug 8

ruiu added a comment to D65995: [ELF] Don't special case symbolic relocations with 0 addend to ifunc in writable locations.

Currently the following 3 relocation types do not trigger the creation of a canonical PLT:

  • GOT-generating
  • PLT-generating
  • symbolic relocation with 0 addend in a writable location
Thu, Aug 8, 11:57 PM · Restricted Project
ruiu added a comment to D65909: ELF: Move sections referred to by __start_/__stop_ symbols into the main partition..

LGTM

Thu, Aug 8, 8:20 PM · Restricted Project
ruiu added a comment to D65875: [ELF] For VS-style diagnostics, prefer using the full path..

Generally looking good, but do these tests pass on Windows on which a path separator is \? It seems like you hardcoded / as a path separator in the tests.

Thu, Aug 8, 2:58 AM · Restricted Project, lld
ruiu added a comment to D65903: [LLD] [COFF] Allow using custom .edata from input object files.

I agree with you that this implementation is straightforward and simple, and I'm fine to add this if changing wine is not easy.

Thu, Aug 8, 2:53 AM · Restricted Project
ruiu added inline comments to D65909: ELF: Move sections referred to by __start_/__stop_ symbols into the main partition..
Thu, Aug 8, 2:15 AM · Restricted Project
ruiu added a comment to D65903: [LLD] [COFF] Allow using custom .edata from input object files.

Is this due to the historical reason that wine is creating an .edata section instead of -export directive? I feel like this feature is obscure, and if there's a way to avoid adding this, I'd like to try that first. If GNU ld is fine with -export directive now, and wine's source code can be updated to use the directive (which is I believe a good refactoring as it would simplify the code), can wine's code be updated?

Thu, Aug 8, 1:18 AM · Restricted Project
ruiu added a comment to D65916: [lld][WebAssembly] Support for growable tables.

I feel that the help message and the commit message are a bit too terse. Could you add a bit more about what is the "table" here? If what that"table" means is obvious for most wasm linker users, we probably don't need to explain it, but I don't believe that's the case.

Thu, Aug 8, 1:04 AM · Restricted Project
ruiu accepted D65911: [lld][WebAssembly] Use createGlobalVariable helper function. NFC..

LGTM

Thu, Aug 8, 1:02 AM · Restricted Project
ruiu accepted D65920: [lld][WebAssembly] Add optional symbols after input file handling.

LGTM

Thu, Aug 8, 1:02 AM · Restricted Project
ruiu added inline comments to D65909: ELF: Move sections referred to by __start_/__stop_ symbols into the main partition..
Thu, Aug 8, 1:02 AM · Restricted Project
ruiu accepted D65882: [LLD][ELF][AArch64] Support for movz, movk tprel relocations.

LGTM

Thu, Aug 8, 12:53 AM · Restricted Project
ruiu added inline comments to D65922: [lld][WebAssembly] Allow linking of PIC code into static binaries.
Thu, Aug 8, 12:53 AM · Restricted Project
ruiu committed rG6fd13f084957: [diagtool] Use `operator<<(Colors)` to print out colored output. (authored by ruiu).
[diagtool] Use `operator<<(Colors)` to print out colored output.
Thu, Aug 8, 12:08 AM
ruiu committed rL368259: [diagtool] Use `operator<<(Colors)` to print out colored output..
[diagtool] Use `operator<<(Colors)` to print out colored output.
Thu, Aug 8, 12:04 AM
ruiu closed D65854: [diagtool] Use `operator<<(Colors)` to print out colored output..
Thu, Aug 8, 12:04 AM · Restricted Project, Restricted Project

Wed, Aug 7

ruiu added inline comments to D65854: [diagtool] Use `operator<<(Colors)` to print out colored output..
Wed, Aug 7, 11:58 PM · Restricted Project, Restricted Project
ruiu added a comment to D65722: [ELF] Expand regions for gaps due to explicit address.

I think this makes sense. Let me submit this tomorrow (I'm leaving now, and I don't want to take a risk to break buildbots.)

Wed, Aug 7, 3:30 AM · Restricted Project
ruiu committed rGe6a33e1f11bf: Handle /align option. (authored by ruiu).
Handle /align option.
Wed, Aug 7, 3:17 AM
ruiu added inline comments to D65736: Handle /align option..
Wed, Aug 7, 3:17 AM · Restricted Project
ruiu committed rL368145: Handle /align option..
Handle /align option.
Wed, Aug 7, 3:15 AM
ruiu closed D65736: Handle /align option..
Wed, Aug 7, 3:15 AM · Restricted Project
ruiu committed rG6c5fc94093d3: Simplify error message output. NFC. (authored by ruiu).
Simplify error message output. NFC.
Wed, Aug 7, 3:12 AM
ruiu committed rL368144: Simplify error message output. NFC..
Simplify error message output. NFC.
Wed, Aug 7, 3:11 AM
ruiu closed D65855: Simplify error message output. NFC..
Wed, Aug 7, 3:11 AM · Restricted Project
ruiu updated the diff for D65855: Simplify error message output. NFC..
  • fix another typo
Wed, Aug 7, 2:58 AM · Restricted Project
ruiu updated the diff for D65855: Simplify error message output. NFC..
  • fix typo
Wed, Aug 7, 2:57 AM · Restricted Project
ruiu created D65855: Simplify error message output. NFC..
Wed, Aug 7, 2:55 AM · Restricted Project
ruiu created D65854: [diagtool] Use `operator<<(Colors)` to print out colored output..
Wed, Aug 7, 2:37 AM · Restricted Project, Restricted Project
ruiu accepted D65810: [ELF] Fix splitting messages for duplicate symbols..

LGTM

Wed, Aug 7, 2:12 AM · Restricted Project, lld
ruiu committed rGcac8df1ab952: Re-submit r367649: Improve raw_ostream so that you can "write" colors using… (authored by ruiu).
Re-submit r367649: Improve raw_ostream so that you can "write" colors using…
Wed, Aug 7, 1:09 AM
ruiu committed rL368131: Re-submit r367649: Improve raw_ostream so that you can "write" colors using….
Re-submit r367649: Improve raw_ostream so that you can "write" colors using…
Wed, Aug 7, 1:08 AM

Tue, Aug 6

ruiu added inline comments to D65736: Handle /align option..
Tue, Aug 6, 5:57 AM · Restricted Project
ruiu accepted D65584: [ELF] Make binding (weak or non-weak) logic consistent for Undefined and SharedSymbol.

Honestly I don't have a strong preference on the behavior of weak symbols in this case. I have a slight concern that if there is a program that depends on the current behavior, they may break by this change, but this case is too subtle and linker-dependent, so I'm perhaps worried too much.

Tue, Aug 6, 5:55 AM · Restricted Project
ruiu accepted D65242: [ELF] More dynamic relocation packing.

This function accumulated code and become a bit too long. We probably have to split the function into small pieces as a follow-up patch.

Tue, Aug 6, 5:46 AM · Restricted Project
ruiu requested changes to D65242: [ELF] More dynamic relocation packing.

Sorry, I forgot one thing: can you add a test?

Tue, Aug 6, 5:46 AM · Restricted Project
ruiu updated the summary of D65736: Handle /align option..
Tue, Aug 6, 5:42 AM · Restricted Project
ruiu added a comment to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..

As to post-process -verbose output, I think that that is too fragile to depend on, as -verbose is mainly for inspection and debugging. Any arbitrary output may be added to the -verbose output that can confuse post-process scripts. I also don't want to recommend users post-process -verbose output because of the same reason -- if people rely on the current -verbose output too much, any change to the file may break scripts in the wild which could prevent us from adding new messages there.

Tue, Aug 6, 5:38 AM · Restricted Project
ruiu accepted D65759: [ELF][ARM] Fix /DISCARD/ of section with .ARM.exidx section.

LGTM

Tue, Aug 6, 5:30 AM · Restricted Project

Mon, Aug 5

ruiu accepted D65716: [ELF] Consistently prioritize non-* wildcards overs "*" in version scripts.

LGTM

Mon, Aug 5, 6:46 AM · Restricted Project
ruiu added a comment to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..

My understanding is that many developers use makefile/ninja generation systems such as cmake rather than hand-write the file themselves. As such would this get much use unless it was integrated into these generators? May be worth approaching them to see if they have any requirements/observations about the option?

Having it integrated into e.g. cmake would be a great project for someone to take on, but for us (FreeBSD) it would be useful by itself. Our build is about 175K lines of hand-written Makefiles and we could plug it in with a small change in a couple of places. In any case we shouldn't hold up lld support waiting on prospective cmake changes IMO.

Ed, does ld.lld a.o -o /dev/null | sed 's/(.*//' | sort -u | sed '$!s/$/ \\/';} generate a usable Makefile fragment? I think the scenarios where people write Makefile are not very common now (think cmake/bazel/meson/scons/buck/...). Makefile and build.ninja are mostly used as an "assembly language". I think a dependency graph feature will be useful, but a Makefile fragment is probably not the best format. (I actually like Makefile more than the alternatives but I know the trend is that people are moving away from hand-written Makefile) At least it looks like the output can be easily generated from ld.lld -t output.

Mon, Aug 5, 6:12 AM · Restricted Project
ruiu added inline comments to D65716: [ELF] Consistently prioritize non-* wildcards overs "*" in version scripts.
Mon, Aug 5, 6:04 AM · Restricted Project
ruiu created D65736: Handle /align option..
Mon, Aug 5, 1:50 AM · Restricted Project

Sun, Aug 4

ruiu accepted D65728: [LLD] [MinGW] Add an lld specific option for requesting to delay load libraries.

LGTM

Sun, Aug 4, 7:18 PM · Restricted Project
ruiu accepted D65727: [LLD] [COFF] Omit automatically imported symbols from the symbol table.

LGTM

Sun, Aug 4, 5:40 PM · Restricted Project

Fri, Aug 2

ruiu accepted D65639: [MachO] Update LLD to use 64-bit offsets with DataExtractor (3/5).

LGTM

Fri, Aug 2, 2:36 AM · Restricted Project, lld
ruiu committed rG4d41c332ef57: Revert r367649: Improve raw_ostream so that you can "write" colors using… (authored by ruiu).
Revert r367649: Improve raw_ostream so that you can "write" colors using…
Fri, Aug 2, 12:25 AM
ruiu committed rL367658: Revert r367649: Improve raw_ostream so that you can "write" colors using….
Revert r367649: Improve raw_ostream so that you can "write" colors using…
Fri, Aug 2, 12:22 AM

Thu, Aug 1

ruiu committed rGc1981b2b269d: Add an assert() to catch possible regexp errors. (authored by ruiu).
Add an assert() to catch possible regexp errors.
Thu, Aug 1, 10:12 PM
ruiu committed rL367651: Add an assert() to catch possible regexp errors..
Add an assert() to catch possible regexp errors.
Thu, Aug 1, 10:10 PM
ruiu committed rG96a7a225f5f9: Add a comment for --vs-diagnostics. (authored by ruiu).
Add a comment for --vs-diagnostics.
Thu, Aug 1, 10:05 PM
ruiu committed rL367650: Add a comment for --vs-diagnostics..
Add a comment for --vs-diagnostics.
Thu, Aug 1, 10:05 PM
ruiu committed rGa52f982f1cd9: Improve raw_ostream so that you can "write" colors using operator<< (authored by ruiu).
Improve raw_ostream so that you can "write" colors using operator<<
Thu, Aug 1, 9:49 PM
ruiu committed rL367649: Improve raw_ostream so that you can "write" colors using operator<<.
Improve raw_ostream so that you can "write" colors using operator<<
Thu, Aug 1, 9:49 PM
ruiu closed D65564: Improve raw_ostream so that you can "write" colors using operator<<.
Thu, Aug 1, 9:48 PM · Restricted Project, Restricted Project
ruiu accepted D65598: [LLD] [COFF] Avoid loading objects for mingw autoimport, when a defined alias exists.

LGTM

Thu, Aug 1, 9:45 PM · Restricted Project
ruiu updated the diff for D65564: Improve raw_ostream so that you can "write" colors using operator<<.
  • rebased
Thu, Aug 1, 8:11 PM · Restricted Project, Restricted Project
ruiu added inline comments to D65564: Improve raw_ostream so that you can "write" colors using operator<<.
Thu, Aug 1, 8:11 PM · Restricted Project, Restricted Project
ruiu committed rG966b9a3b9d0e: Fix an unused variable warning. (authored by ruiu).
Fix an unused variable warning.
Thu, Aug 1, 7:52 PM
ruiu committed rL367643: Fix an unused variable warning..
Fix an unused variable warning.
Thu, Aug 1, 7:50 PM
ruiu accepted D65565: [LLD] [COFF] Fix wholearchive with thin archives.

LGTM

Thu, Aug 1, 2:34 AM · Restricted Project
ruiu created D65564: Improve raw_ostream so that you can "write" colors using operator<<.
Thu, Aug 1, 2:21 AM · Restricted Project, Restricted Project
ruiu accepted D64903: [ELF] Add -z separate-code and pad the last page of last PF_X PT_LOAD with traps only if -z separate-code is specified.

LGTM

Thu, Aug 1, 2:05 AM · Restricted Project

Wed, Jul 31

ruiu accepted D65213: [ELF] With --vs-diagnostics, print a separate message for each location of a duplicate symbol..

LGTM

Wed, Jul 31, 11:18 PM · Restricted Project
ruiu added inline comments to D65213: [ELF] With --vs-diagnostics, print a separate message for each location of a duplicate symbol..
Wed, Jul 31, 9:29 PM · Restricted Project
ruiu accepted D65499: [ELF] Fix finding the location in messages for undefined hidden symbols..

LGTM

Wed, Jul 31, 6:23 PM · lld, Restricted Project
ruiu added inline comments to D65213: [ELF] With --vs-diagnostics, print a separate message for each location of a duplicate symbol..
Wed, Jul 31, 6:23 PM · Restricted Project

Tue, Jul 30

ruiu added inline comments to D65499: [ELF] Fix finding the location in messages for undefined hidden symbols..
Tue, Jul 30, 11:59 PM · lld, Restricted Project
ruiu retitled D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD. from Add `--write-dependencies` option, which is equivalent to compiler option -MD. to Add `--dependency-files` option, which is equivalent to compiler option -MD..
Tue, Jul 30, 6:55 PM · Restricted Project
ruiu updated the diff for D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..
  • address review comments
Tue, Jul 30, 6:55 PM · Restricted Project
ruiu added inline comments to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..
Tue, Jul 30, 6:55 PM · Restricted Project
ruiu added inline comments to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..
Tue, Jul 30, 6:49 PM · Restricted Project
ruiu added a comment to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..

No objections to the idea in principle. I think it may need a bit of wider discussion to reach its full potential though. My understanding is that many developers use makefile/ninja generation systems such as cmake rather than hand-write the file themselves. As such would this get much use unless it was integrated into these generators? May be worth approaching them to see if they have any requirements/observations about the option?

Tue, Jul 30, 5:30 PM · Restricted Project
ruiu added inline comments to D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..
Tue, Jul 30, 2:52 AM · Restricted Project

Mon, Jul 29

ruiu updated the diff for D65430: Add `--dependency-files` option, which is equivalent to compiler option -MD..
  • quote a filename if it contains a space character
  • add a test
Mon, Jul 29, 11:46 PM · Restricted Project