ruiu (Rui Ueyama)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 18 2013, 12:16 AM (218 w, 3 d)

Recent Activity

Yesterday

ruiu accepted D34594: [GSoC] Delete space after flags which has '=' prefix.

LGTM

Sat, Jun 24, 9:02 AM
ruiu added inline comments to D34594: [GSoC] Delete space after flags which has '=' prefix.
Sat, Jun 24, 7:49 AM

Fri, Jun 23

ruiu accepted D34577: [COFF] Improve synthetic symbol handling.

LGTM

Fri, Jun 23, 3:50 PM
ruiu added a comment to D34577: [COFF] Improve synthetic symbol handling.

Overall looking good, a few minor comments.

Fri, Jun 23, 3:39 PM
ruiu accepted D34525: Replace trivial use of external rc.exe by writing our own .res file..

LGTM

Fri, Jun 23, 11:29 AM
ruiu accepted D34558: [GSoC] Add support for CC1 options..

LGTM

Fri, Jun 23, 9:51 AM
ruiu added a comment to D34204: [LLD][LinkerScript] Allow non-alloc sections to be assigned to segments..

This is probably OK but I'd like to hear from Rafael as he wrote the code you are updating.

Fri, Jun 23, 9:42 AM
ruiu added inline comments to D34558: [GSoC] Add support for CC1 options..
Fri, Jun 23, 9:39 AM
ruiu added a comment to D34525: Replace trivial use of external rc.exe by writing our own .res file..

Looks much better. A few minor comments.

Fri, Jun 23, 8:44 AM
ruiu committed rL306116: Sort the autocomplete candidates before printing them out..
Sort the autocomplete candidates before printing them out.
Fri, Jun 23, 8:38 AM
ruiu closed D34557: Sort the autocomplete candidates before printing them out. by committing rL306116: Sort the autocomplete candidates before printing them out..
Fri, Jun 23, 8:38 AM
ruiu added a comment to D34554: [ELF] FIx use-after-return of archive path .

I think a better approach would be to change the type of ArchiveName from StringRef to std::string. This patch may fix the issue, but I'm not 100% sure because it fixes only one path that can reach addFile function. There might be some other places at which we pass on-stack objects to the function.

Fri, Jun 23, 8:21 AM · lld
ruiu created D34557: Sort the autocomplete candidates before printing them out..
Fri, Jun 23, 8:16 AM

Thu, Jun 22

ruiu added a comment to D34525: Replace trivial use of external rc.exe by writing our own .res file..

Well, operator<< should still work, but if doing that makes code messy, you don't have to do that.

Thu, Jun 22, 4:29 PM
ruiu accepted D34541: [COFF] Fix SECTION and SECREL relocation handling for absolute symbols.

LGTM

Thu, Jun 22, 4:27 PM
ruiu added inline comments to D34525: Replace trivial use of external rc.exe by writing our own .res file..
Thu, Jun 22, 3:56 PM
ruiu accepted D34539: COFF: handle "undef - ." expressions.

LGTM

Thu, Jun 22, 3:18 PM
ruiu added inline comments to D33680: [ELF] - Resolve references properly when using .symver directive.
Thu, Jun 22, 3:08 PM
ruiu added a comment to D34525: Replace trivial use of external rc.exe by writing our own .res file..

It indeed will require extra copy, but that is fine. You don't need to avoid extra copy here since the generated file image is small and this is executed just once.

Thu, Jun 22, 2:51 PM
ruiu accepted D34530: Change creation of relative relocations on COFF.

LGTM

Thu, Jun 22, 2:07 PM
ruiu added a comment to D34525: Replace trivial use of external rc.exe by writing our own .res file..

I think a better way of creating manifest file contents is to construct a temporary std::vector and then copy it to a MemoryBuffer after you append everything to the vector. This way, you don't need to compute the size of the result ahead of time.

Thu, Jun 22, 1:30 PM
ruiu committed rL306036: Keep the original symbol name when renamed..
Keep the original symbol name when renamed.
Thu, Jun 22, 10:31 AM
ruiu added a comment to D34355: [LLD][ELF] Define _GLOBAL_OFFSET_TABLE_ to base of .got for ARM.

Yes, overall looking good, but I have one question.

Thu, Jun 22, 9:41 AM

Wed, Jun 21

ruiu added inline comments to D33630: [ELF] Fill the empty space in executable segments with instruction padding.
Wed, Jun 21, 2:30 PM · lld
ruiu accepted D34465: ELF: Don't dereference Repl in MarkLive. NFCI..

LGTM

Wed, Jun 21, 2:21 PM
ruiu added a comment to D34476: [COFF] Allow comdat discarding and GC of non-section chunks.

LGTM given that you are going to enable GC for other chunks.

Wed, Jun 21, 2:19 PM
ruiu added a comment to D34459: [ELF] Better handling of _GLOBAL_OFFSET_TABLE_.

I don't have any further comments, but can you please update the patch?

Wed, Jun 21, 10:36 AM · lld
ruiu accepted D34432: [PDB] Add symbols to the PDB.

LGTM

Wed, Jun 21, 10:16 AM
ruiu committed rL305930: Use -NOT prefix instead of adding `not` to FileCheck..
Use -NOT prefix instead of adding `not` to FileCheck.
Wed, Jun 21, 9:51 AM
ruiu closed D34435: Use -NOT prefix instead of adding `not` to FileCheck. by committing rL305930: Use -NOT prefix instead of adding `not` to FileCheck..
Wed, Jun 21, 9:51 AM
ruiu committed rL305929: [COFF] Set MajorLinkerVersion to 14 instead of 0..
[COFF] Set MajorLinkerVersion to 14 instead of 0.
Wed, Jun 21, 9:43 AM
ruiu added inline comments to D34432: [PDB] Add symbols to the PDB.
Wed, Jun 21, 9:26 AM
ruiu added a comment to D34459: [ELF] Better handling of _GLOBAL_OFFSET_TABLE_.

Do you have any program that actually needs this change?

Wed, Jun 21, 9:22 AM · lld
ruiu added a comment to D13392: [ELF2] - Implemented --exclude-libs flag.

I landed https://reviews.llvm.org/rL305920 to add the -exclude-libs flag.

Wed, Jun 21, 8:39 AM
ruiu committed rL305920: Implement the --exclude-libs option..
Implement the --exclude-libs option.
Wed, Jun 21, 8:37 AM
ruiu closed D34422: Implement the --exclude-libs option. by committing rL305920: Implement the --exclude-libs option..
Wed, Jun 21, 8:37 AM
ruiu added a comment to D34232: [ELF] Fix writing the content of the .got section in a wrong place..

It looks like it's not the only problem for .got but for all sections. How does it work for other sections?

Wed, Jun 21, 7:26 AM · lld
ruiu added inline comments to D34422: Implement the --exclude-libs option..
Wed, Jun 21, 6:47 AM
ruiu added a comment to D34326: [ELF] - Support SORT_BY_INIT_PRIORITY for .ctors.*/.dtors sections in linkerscript..

I wouldn't do anything for it unless an evidence that that is actually useful is shown.

Wed, Jun 21, 6:46 AM
ruiu accepted D34442: [ELF] Add an apostrophe after a file name when reporting discarded sections..

LGTM

Wed, Jun 21, 6:45 AM · lld

Tue, Jun 20

ruiu committed rL305877: Remove redundant namespace specifier..
Remove redundant namespace specifier.
Tue, Jun 20, 8:05 PM
ruiu added inline comments to D33680: [ELF] - Resolve references properly when using .symver directive.
Tue, Jun 20, 7:53 PM
ruiu added a comment to D34326: [ELF] - Support SORT_BY_INIT_PRIORITY for .ctors.*/.dtors sections in linkerscript..

The rule this patch implements seems too odd that I don't think we should handle this. If you want to sort .ctors or .dtors, I think you can use SORT_BY_NAME. If you use SORT_BY_INIT_PRIORITY on .ctors or .dtors, that is a misuse of the feature.

Tue, Jun 20, 7:45 PM
ruiu added a comment to D34204: [LLD][LinkerScript] Allow non-alloc sections to be assigned to segments..

Can you change your program?

Tue, Jun 20, 7:39 PM
ruiu added inline comments to D34432: [PDB] Add symbols to the PDB.
Tue, Jun 20, 7:34 PM
ruiu committed rL305876: Define __guard_{iat,longjmp}_{count,table} symbols..
Define __guard_{iat,longjmp}_{count,table} symbols.
Tue, Jun 20, 7:27 PM
ruiu created D34435: Use -NOT prefix instead of adding `not` to FileCheck..
Tue, Jun 20, 6:15 PM
ruiu committed rL305868: Improve error messages..
Improve error messages.
Tue, Jun 20, 4:12 PM
ruiu updated the summary of D34422: Implement the --exclude-libs option..
Tue, Jun 20, 2:55 PM
ruiu added a comment to D13392: [ELF2] - Implemented --exclude-libs flag.

I re-implemented the feature in https://reviews.llvm.org/D34422. The new implementation uses a different strategy than this patch, and by utilizing the relationship of archive files and object files, the new implementation is much simpler and less intrusive than this one.

Tue, Jun 20, 2:47 PM
ruiu created D34422: Implement the --exclude-libs option..
Tue, Jun 20, 2:45 PM
ruiu accepted D34413: Fix argument numbering in OPTION macro.

LGTM. Thank you for fixing this!

Tue, Jun 20, 12:16 PM
ruiu added a comment to D13392: [ELF2] - Implemented --exclude-libs flag.

If Android needs the flag, I'm happy to re-implement this feature, but can you give me a pointer where the flag is used in Android? I want to understand how the flag is being used before implementing it.

Tue, Jun 20, 12:01 PM

Mon, Jun 19

ruiu added inline comments to D33630: [ELF] Fill the empty space in executable segments with instruction padding.
Mon, Jun 19, 6:25 PM · lld
ruiu accepted D34356: [PDB] Don't emit debug info associated with dead chunks.

LGTM

Mon, Jun 19, 1:54 PM
ruiu accepted D34358: LLD: Move ELF/Mips.cpp to ELF/Arch/MipsArchTree.cpp.

LGTM

Mon, Jun 19, 1:50 PM
ruiu added inline comments to D34356: [PDB] Don't emit debug info associated with dead chunks.
Mon, Jun 19, 1:34 PM
ruiu added a comment to D34204: [LLD][LinkerScript] Allow non-alloc sections to be assigned to segments..

Hi Andrew,

Mon, Jun 19, 12:36 PM
ruiu added inline comments to D34356: [PDB] Don't emit debug info associated with dead chunks.
Mon, Jun 19, 12:33 PM
ruiu added a comment to D34358: LLD: Move ELF/Mips.cpp to ELF/Arch/MipsArchTree.cpp.

How about renaming the file MipsArchTree.cpp or something like that?

Mon, Jun 19, 12:30 PM
ruiu added a comment to D34358: LLD: Move ELF/Mips.cpp to ELF/Arch/MipsArchTree.cpp.

I think we shouldn't do this. It might make sense to move this file under Arch, but merging it with Arch/Mips.cpp decreases readability as the file now contains totally unrelated functions.

Mon, Jun 19, 11:49 AM
ruiu added inline comments to D34356: [PDB] Don't emit debug info associated with dead chunks.
Mon, Jun 19, 11:44 AM
ruiu committed rL305726: Fix build breakage..
Fix build breakage.
Mon, Jun 19, 11:05 AM
ruiu committed rL305718: Fix a threading bug..
Fix a threading bug.
Mon, Jun 19, 10:47 AM
ruiu added a comment to D34326: [ELF] - Support SORT_BY_INIT_PRIORITY for .ctors.*/.dtors sections in linkerscript..

I don't know if we need this, as it implements a feature that is different from what the manual says, and no one is complaining about this.

Mon, Jun 19, 10:31 AM
ruiu committed rL305714: Replace CRLF with LF..
Replace CRLF with LF.
Mon, Jun 19, 10:23 AM
ruiu accepted D34257: [PDB] Start emitting source file and line information.

LGTM

Mon, Jun 19, 10:09 AM
ruiu added inline comments to D33383: [GSoC] Flag value completion for clang.
Mon, Jun 19, 9:59 AM
ruiu added a comment to D34203: [LLD][LinkerScript] Add support for segment NONE..

Yes

Mon, Jun 19, 6:49 AM
ruiu accepted D33383: [GSoC] Flag value completion for clang.

LGTM

Mon, Jun 19, 6:16 AM

Sun, Jun 18

ruiu added a comment to D33383: [GSoC] Flag value completion for clang.

This is beautiful. Overall looking pretty good. Some minor stylistic comments.

Sun, Jun 18, 8:53 AM

Sat, Jun 17

ruiu accepted D34313: Prefer -Ttext over linker script values.

LGTM

Sat, Jun 17, 4:21 PM
ruiu added inline comments to D33383: [GSoC] Flag value completion for clang.
Sat, Jun 17, 1:55 PM
ruiu added a comment to D33383: [GSoC] Flag value completion for clang.

As to the order of Command variable contents, I think I'm not convinced. You can access the last element as Command[Command.size() - 1] (or maybe Command.back()), no? It is slightly awkward than Command[0], but that's not horribly hard to handle, and passing arguments in the reverse order seems more counter-intuitive to me.

Sat, Jun 17, 1:37 PM

Fri, Jun 16

ruiu accepted D34307: Have writeCOFFWriter return Expected<unique_ptr>..

LGTM

Fri, Jun 16, 7:12 PM
ruiu added inline comments to D34307: Have writeCOFFWriter return Expected<unique_ptr>..
Fri, Jun 16, 5:54 PM
ruiu accepted D34302: [lld] Remove /debugpdb option from LLD.

LGTM

Fri, Jun 16, 4:28 PM
ruiu added a comment to D33630: [ELF] Fill the empty space in executable segments with instruction padding.

Doesn't something like work? https://github.com/rui314/llvm-project/commit/5fcc112a76edc12aa58eeb961ba6749525ab11f8

Fri, Jun 16, 4:21 PM · lld
ruiu committed rL305601: Update a comment..
Update a comment.
Fri, Jun 16, 3:45 PM
ruiu accepted D34265: Switch external cvtres.exe for llvm's own resource library. In this patch, I flip the switch in DriverUtils from using the external cvtres.exe tool to using the Windows Resource library in llvm. I also fixed a bug where .rsrc sections were....

LGTM

Fri, Jun 16, 1:59 PM
ruiu committed rL305577: Do not use make<> to allocate TargetInfo. NFC..
Do not use make<> to allocate TargetInfo. NFC.
Fri, Jun 16, 1:15 PM
ruiu accepted D34288: [COFF] Drop unused comdat sections when GC is turned off.

LGTM

Fri, Jun 16, 12:55 PM
ruiu committed rL305567: Add comments for AVR support..
Add comments for AVR support.
Fri, Jun 16, 10:54 AM
ruiu added a comment to D34265: Switch external cvtres.exe for llvm's own resource library. In this patch, I flip the switch in DriverUtils from using the external cvtres.exe tool to using the Windows Resource library in llvm. I also fixed a bug where .rsrc sections were....

The first line of a commit message should be shorter than that is now. If you look at other commits, you'll find that their first lines are usually less than 80 or 100 characters.

Fri, Jun 16, 10:42 AM
ruiu committed rL305565: Split Target.cpp into small files..
Split Target.cpp into small files.
Fri, Jun 16, 10:33 AM
ruiu closed D34222: Split Target.cpp into small files. by committing rL305565: Split Target.cpp into small files..
Fri, Jun 16, 10:33 AM
ruiu added inline comments to D34257: [PDB] Start emitting source file and line information.
Fri, Jun 16, 9:29 AM
ruiu added a comment to D34222: Split Target.cpp into small files..

Rafael, are you ok with this?

Fri, Jun 16, 9:15 AM

Thu, Jun 15

ruiu accepted D34270: Clean up some things in the WindowsResource changes..

LGTM

Thu, Jun 15, 9:04 PM
ruiu added a comment to D34265: Switch external cvtres.exe for llvm's own resource library. In this patch, I flip the switch in DriverUtils from using the external cvtres.exe tool to using the Windows Resource library in llvm. I also fixed a bug where .rsrc sections were....

As a convention, please write a short summary (ideally less than 80 chars) on the first line of a commit message, so that they are not wrapped around
... like this.

Thu, Jun 15, 7:55 PM
ruiu committed rL305542: Fix buildbots..
Fix buildbots.
Thu, Jun 15, 7:43 PM
ruiu committed rL305541: Fix msan buildbot..
Fix msan buildbot.
Thu, Jun 15, 7:18 PM
ruiu added inline comments to D34257: [PDB] Start emitting source file and line information.
Thu, Jun 15, 6:27 PM
ruiu added inline comments to D34257: [PDB] Start emitting source file and line information.
Thu, Jun 15, 4:33 PM
ruiu added a comment to D33630: [ELF] Fill the empty space in executable segments with instruction padding.

OK, that makes sense. But can you simplify the implementation? It seems this patch changes too many places.

Thu, Jun 15, 4:09 PM · lld
ruiu added a comment to D33630: [ELF] Fill the empty space in executable segments with instruction padding.

If it is not loaded and end of a segment is filled with zeros in memory, it is not a desired result for you, no?

Thu, Jun 15, 3:50 PM · lld
ruiu updated the diff for D34222: Split Target.cpp into small files..
  • Rename arch Arch
  • Address review comments.
Thu, Jun 15, 3:32 PM
ruiu added inline comments to D34222: Split Target.cpp into small files..
Thu, Jun 15, 3:29 PM
ruiu added a comment to D33630: [ELF] Fill the empty space in executable segments with instruction padding.

In response to ruii's last comment:

I don't think it follows that any *section*'s size should be rounded up. As I just said, that perverts the intent of the link to suggest that the fill bytes are actually part of some section, which they are not.

You may have a case for saying that the *segment*'s size, i.e. p_filesz/p_memsz in the phdr, should be rounded to the page size.
However, given ELF's general statement that up to the whole p_align-aligned area *might* be visible at runtime, I don't really agree with the idea that p_filesz should represent the entire length that we think might be visible.
I think it is better that p_filesz represents the part of the file that *must* be visible at runtime for the program's intent to be correct.
If there were some system or hardware that could choose the accessibility at byte-granularity, then it would be fine and good for it to make only the portion of the file up to p_filesz accessible.

Thu, Jun 15, 3:21 PM · lld
ruiu added a comment to D33630: [ELF] Fill the empty space in executable segments with instruction padding.

I think I understand the points you made, and I partially agree with that, but we can see it from a different perspective. We fill the gap after executable segments with trap instructions, with the clear intention that the trap instructions will be loaded to memory. So in that sense it makes sense to round up size of executable sections to a page size. Also, in practice, this code seems too large to do a simple thing we've already done for filling gaps between sections. We already have code to fill gaps in LinkerScript.cpp, and this new code seems almost the same.

Thu, Jun 15, 3:01 PM · lld