Page MenuHomePhabricator

grimar (George Rimar)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 21 2015, 12:36 AM (190 w, 6 d)

Recent Activity

Thu, May 16

grimar accepted D61941: [llvm-objdump]Improve testing of some switches #1.

LGTM

Thu, May 16, 5:06 AM · Restricted Project
grimar added a comment to D61941: [llvm-objdump]Improve testing of some switches #1.

I only have a minor comment (inlined).

Out of curiosity are you doing some code coverage check on the llvm-objdump to find the places uncovered?
Asking because I did that few times for LLD and this gave interesting results that time (IMO). I added many test cases and found few interesting bugs and a dead code pieces.
(i.e. if not - I can try to do that and share the results).

I've not been doing code coverage checks as such, but I have been inspecting the code to see what behaviour each feature has (and sometimes cross-referencing with GNU). A number of the test cases in this and the following patches that I'm still preparing are based on an internal test suite we have for a proprietary tool.

Thu, May 16, 3:16 AM · Restricted Project
grimar added a comment to D61941: [llvm-objdump]Improve testing of some switches #1.

I only have a minor comment (inlined).

Thu, May 16, 1:30 AM · Restricted Project

Wed, May 15

grimar committed rGec895f11cee1: [llvm-readobj] - Revert r360676 partially. NFC. (authored by grimar).
[llvm-readobj] - Revert r360676 partially. NFC.
Wed, May 15, 11:22 PM
grimar committed rL360860: [llvm-readobj] - Revert r360676 partially. NFC..
[llvm-readobj] - Revert r360676 partially. NFC.
Wed, May 15, 11:22 PM
grimar accepted D61895: Introduce CommonSymbol..

Generally it looks good to me.

Wed, May 15, 6:17 AM · Restricted Project
grimar accepted D61857: [LTO] Improve readability of module IDs.

LGTM

Wed, May 15, 5:03 AM · Restricted Project
grimar added inline comments to D61895: Introduce CommonSymbol..
Wed, May 15, 4:16 AM · Restricted Project
grimar created D61937: [llvm-readelf] - Rework how we parse the .dynamic section..
Wed, May 15, 3:33 AM
grimar added inline comments to D61563: [ELF] Error on relocations to local undefined symbols.
Wed, May 15, 12:54 AM · Restricted Project
grimar accepted D61896: De-template parseFile() and SymbolTable's add-family functions..

LGTM

Wed, May 15, 12:21 AM · Restricted Project

Tue, May 14

grimar committed rG9e88a2686374: [llvm-readobj] - Apply clang format. NFC. (authored by grimar).
[llvm-readobj] - Apply clang format. NFC.
Tue, May 14, 7:21 AM
grimar committed rL360676: [llvm-readobj] - Apply clang format. NFC..
[llvm-readobj] - Apply clang format. NFC.
Tue, May 14, 7:21 AM
grimar added a comment to D61895: Introduce CommonSymbol..

This is interesting, could you please hold this on (in case you'll have LGTM from other person) until I'll look more carefully in my tomorrow (+24h)?

Tue, May 14, 6:46 AM · Restricted Project
grimar accepted D61897: Remove SymbolTable::addBitcode as it is redundant..

LGTM.

Tue, May 14, 6:36 AM · Restricted Project
grimar accepted D61856: [test]Make test work on Windows.

LGTM, this works for me under windows.

Tue, May 14, 2:21 AM · Restricted Project
grimar added a comment to D61857: [LTO] Improve readability of module IDs.

My minor concern is that the new message is a bit inconsistent with our other error messages I think.

Tue, May 14, 2:15 AM · Restricted Project

Mon, May 13

grimar added inline comments to D61855: Simplify SymbolTable::add{Defined,Undefined,...} functions..
Mon, May 13, 7:35 AM · Restricted Project
grimar added inline comments to D61712: [ELF] Initialize Target before it may be referenced by findAux when reporting "duplicate symbol" error.
Mon, May 13, 6:42 AM · Restricted Project
grimar accepted D61854: Move SymbolTable::addFile to InputFiles.cpp..

LGTM

Mon, May 13, 6:37 AM · Restricted Project
grimar added inline comments to D61688: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic) via -zcommon-page-size.
Mon, May 13, 6:24 AM · Restricted Project
grimar added inline comments to D61710: lld: Add a warning limit, similar to the existing error limit.
Mon, May 13, 6:05 AM · Restricted Project
grimar added inline comments to D61688: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic) via -zcommon-page-size.
Mon, May 13, 5:55 AM · Restricted Project
grimar added a comment to D61688: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic) via -zcommon-page-size.

Few minor nits from me.

Mon, May 13, 4:42 AM · Restricted Project
grimar added inline comments to D61117: Fix Bug 41353 - unique symbols printed as D instead of u.
Mon, May 13, 2:20 AM · Restricted Project
grimar added inline comments to D61117: Fix Bug 41353 - unique symbols printed as D instead of u.
Mon, May 13, 2:16 AM · Restricted Project
grimar added inline comments to D60353: ELF: Add basic partition data structures and behaviours..
Mon, May 13, 1:42 AM · Restricted Project
grimar added a comment to D60353: ELF: Add basic partition data structures and behaviours..

This looks good to me, but I do not think I am a proper person to give a final accept.
Minor nits/suggestions are inline.

Mon, May 13, 1:39 AM · Restricted Project

Wed, May 8

grimar added inline comments to D61674: [llvm-objcopy] Improve error message for unrecognised archive member.
Wed, May 8, 7:00 AM · Restricted Project
grimar added inline comments to D61674: [llvm-objcopy] Improve error message for unrecognised archive member.
Wed, May 8, 6:41 AM · Restricted Project
grimar added inline comments to D61674: [llvm-objcopy] Improve error message for unrecognised archive member.
Wed, May 8, 6:00 AM · Restricted Project
grimar accepted D61674: [llvm-objcopy] Improve error message for unrecognised archive member.

LGTM

Wed, May 8, 5:58 AM · Restricted Project
grimar added inline comments to D61674: [llvm-objcopy] Improve error message for unrecognised archive member.
Wed, May 8, 5:35 AM · Restricted Project
grimar committed rG17dbb19f704d: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow… (authored by grimar).
[llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow…
Wed, May 8, 12:29 AM
grimar committed rL360227: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow….
[llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow…
Wed, May 8, 12:29 AM
grimar closed D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names".
Wed, May 8, 12:29 AM · Restricted Project

Tue, May 7

grimar added inline comments to D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names".
Tue, May 7, 7:13 AM · Restricted Project
grimar updated the diff for D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names".
  • Addressed review comment.
Tue, May 7, 7:12 AM · Restricted Project
grimar committed rG5c922f698847: [llvm-objdump] - Print relocation record in a GNU format. (authored by grimar).
[llvm-objdump] - Print relocation record in a GNU format.
Tue, May 7, 6:15 AM
grimar committed rL360143: [llvm-objdump] - Print relocation record in a GNU format..
[llvm-objdump] - Print relocation record in a GNU format.
Tue, May 7, 6:15 AM
grimar closed D61312: [llvm-objdump] - Print relocation record in a GNU format..
Tue, May 7, 6:14 AM · Restricted Project
grimar committed rG72f7a98876fb: [LLD][ELF] - Remove symbol-name-offset.elf binary from test cases. (authored by grimar).
[LLD][ELF] - Remove symbol-name-offset.elf binary from test cases.
Tue, May 7, 5:59 AM
grimar committed rLLD360139: [LLD][ELF] - Remove symbol-name-offset.elf binary from test cases..
[LLD][ELF] - Remove symbol-name-offset.elf binary from test cases.
Tue, May 7, 5:59 AM
grimar committed rL360139: [LLD][ELF] - Remove symbol-name-offset.elf binary from test cases..
[LLD][ELF] - Remove symbol-name-offset.elf binary from test cases.
Tue, May 7, 5:59 AM
grimar added inline comments to D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names".
Tue, May 7, 5:41 AM · Restricted Project
grimar committed rG0974688a42c9: [yaml2obj] - Allow setting st_value explicitly for Symbol. (authored by grimar).
[yaml2obj] - Allow setting st_value explicitly for Symbol.
Tue, May 7, 5:11 AM
grimar committed rL360137: [yaml2obj] - Allow setting st_value explicitly for Symbol..
[yaml2obj] - Allow setting st_value explicitly for Symbol.
Tue, May 7, 5:11 AM
grimar closed D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol..
Tue, May 7, 5:11 AM · Restricted Project
grimar created D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names".
Tue, May 7, 3:40 AM · Restricted Project
grimar added inline comments to D61117: Fix Bug 41353 - unique symbols printed as D instead of u.
Tue, May 7, 2:35 AM · Restricted Project
grimar added a comment to D61563: [ELF] Error on relocations to local undefined symbols.

I also do not know if undefined local symbol can be a real thing.
So my comment is about implementation.

Tue, May 7, 1:39 AM · Restricted Project
grimar added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset).

I don't know if DT_RELACOUNT was a glibc invention or a Sun invention. To be honest, if it is not that we've added it in the first place, I wouldn't suggest adding it :D

Tue, May 7, 12:13 AM · Restricted Project
grimar added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset).

Sorting R_*_RELATIVE relocs first and setting REL(A)COUNT is a format requirement, period. It's not a debatable question of optimization.

Tue, May 7, 12:03 AM · Restricted Project

Mon, May 6

grimar retitled D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol. from [yaml2obj] - Treat integer symbol names as explicit st_name value. to [yaml2obj] - Allow setting st_value explicitly for Symbol..
Mon, May 6, 9:11 AM · Restricted Project
grimar updated the diff for D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol..
  • Reimplemented as was suggested during the review.
Mon, May 6, 9:08 AM · Restricted Project
grimar added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset).

For the PR41692 reproduce tarball, ld.lld @response.txt --pack-dyn-relocs=none (without the patch) shows the relative relocations aren't sorted by r_offset.

# I deleted some other flags to compare the result with gold and ld.bfd
Relocation section '.rela.dyn' at offset 0x14008 contains 65020 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000405000  0000000000000008 R_X86_64_RELATIVE                         1954a5
0000000000405010  0000000000000008 R_X86_64_RELATIVE                         19eb00
000000000049d0f0  0000000000000008 R_X86_64_RELATIVE                         1ee500
000000000049d100  0000000000000008 R_X86_64_RELATIVE                         1ee54f
000000000049d110  0000000000000008 R_X86_64_RELATIVE                         1ee525
0000000000403000  0000000000000008 R_X86_64_RELATIVE                         1ee580
0000000000403008  0000000000000008 R_X86_64_RELATIVE                         1ee5a0
0000000000403010  0000000000000008 R_X86_64_RELATIVE                         0

Relocations sorted by r_offset would make a poor person's life easier when debugging with readelf -r :) (I use readelf -r much while debugging numerous internal issues)

Mon, May 6, 7:16 AM · Restricted Project
grimar accepted D61594: [AMDGPU][test] Define local symbols used in amdgpu-relocs.s.

This LGTM.

Mon, May 6, 7:10 AM · Restricted Project
grimar added inline comments to D61594: [AMDGPU][test] Define local symbols used in amdgpu-relocs.s.
Mon, May 6, 6:54 AM · Restricted Project
grimar added inline comments to D61563: [ELF] Error on relocations to local undefined symbols.
Mon, May 6, 5:53 AM · Restricted Project
grimar added inline comments to D61563: [ELF] Error on relocations to local undefined symbols.
Mon, May 6, 5:42 AM · Restricted Project
grimar added a comment to rL359942: [lld] Specify output file explicitly.

Perhaps even better would be to not create output at all. I.e -o /dev/null.

Mon, May 6, 5:00 AM
grimar added a comment to D61117: Fix Bug 41353 - unique symbols printed as D instead of u.

Looks like support was recently added in rL359380, though without any tests, and was accidentally reverted.

Mon, May 6, 4:37 AM · Restricted Project
grimar added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset).

The sorting behavior is entirely an optimization for locality at dynamic reloc time. Both locality of symtab indices and locality of reloc offsets are useful to that goal.

Mon, May 6, 3:35 AM · Restricted Project
grimar closed D61298: [LLD] Emit dynamic relocations for references to script symbols in -pie links.

Was committed as r359683

Mon, May 6, 1:59 AM · Restricted Project
grimar added a comment to D61312: [llvm-objdump] - Print relocation record in a GNU format..

James, are you OK with this patch?

Mon, May 6, 1:45 AM · Restricted Project

Fri, May 3

grimar added a comment to D61477: [ELF] -z combreloc: sort dynamic relocations by (!is_relative,symbol_index,r_offset).

But why this is an issue? Both PR and this patch description does not answer that it seems.
Is it just a cosmetic change for the output?

Fri, May 3, 8:06 AM · Restricted Project

Thu, May 2

grimar committed rG366212726a1a: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix… (authored by grimar).
[yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix…
Thu, May 2, 12:28 PM
grimar committed rL359818: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix….
[yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix…
Thu, May 2, 12:27 PM
grimar closed D61190: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users..
Thu, May 2, 12:27 PM · Restricted Project
grimar updated the summary of D61190: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users..
Thu, May 2, 12:27 PM · Restricted Project

Wed, May 1

grimar added inline comments to D61190: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users..
Wed, May 1, 4:21 PM · Restricted Project
grimar updated the diff for D61190: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users..
  • Rebased.
Wed, May 1, 4:21 PM · Restricted Project
grimar added inline comments to D61312: [llvm-objdump] - Print relocation record in a GNU format..
Wed, May 1, 8:58 AM · Restricted Project
grimar updated the diff for D61312: [llvm-objdump] - Print relocation record in a GNU format..
  • Addressed review comments.
  • Cosmetic change in the test case.
Wed, May 1, 8:58 AM · Restricted Project
grimar added inline comments to D61312: [llvm-objdump] - Print relocation record in a GNU format..
Wed, May 1, 3:21 AM · Restricted Project
grimar added inline comments to D61322: [yaml2obj] - Report when unknown section is referenced from program header declaration block..
Wed, May 1, 2:49 AM · Restricted Project
grimar committed rGf5345a3f4c7d: [yaml2obj] - Report when unknown section is referenced from program header… (authored by grimar).
[yaml2obj] - Report when unknown section is referenced from program header…
Wed, May 1, 2:46 AM
grimar committed rL359663: [yaml2obj] - Report when unknown section is referenced from program header….
[yaml2obj] - Report when unknown section is referenced from program header…
Wed, May 1, 2:45 AM
grimar closed D61322: [yaml2obj] - Report when unknown section is referenced from program header declaration block..
Wed, May 1, 2:45 AM · Restricted Project
grimar accepted D61201: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic).

LGTM with a nit.

Wed, May 1, 2:37 AM

Tue, Apr 30

grimar requested review of D61322: [yaml2obj] - Report when unknown section is referenced from program header declaration block..
Tue, Apr 30, 10:29 AM · Restricted Project
grimar updated the diff for D61322: [yaml2obj] - Report when unknown section is referenced from program header declaration block..

Sorry, during final test run I found this fix reveals a bug in one of the testcases: llvm-objcopy/ELF/no-build-id.test
I fixed it. Could you review this change please too?

Tue, Apr 30, 10:29 AM · Restricted Project
grimar added a comment to D61190: [yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users..

I do wonder how you made sure you caught all instances!

Tue, Apr 30, 9:47 AM · Restricted Project
grimar created D61322: [yaml2obj] - Report when unknown section is referenced from program header declaration block..
Tue, Apr 30, 9:47 AM · Restricted Project
grimar created D61312: [llvm-objdump] - Print relocation record in a GNU format..
Tue, Apr 30, 7:16 AM · Restricted Project
grimar accepted D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

LGTM.

Tue, Apr 30, 6:46 AM · Restricted Project
grimar added a comment to D60825: [llvm-objcopy] - Check dynamic relocation sections for broken references..

I submitted a bug discussed here: https://bugs.llvm.org/show_bug.cgi?id=41669

Tue, Apr 30, 4:10 AM · Restricted Project
grimar committed rG67f590e2867a: [llvm-objcopy] - Check dynamic relocation sections for broken references. (authored by grimar).
[llvm-objcopy] - Check dynamic relocation sections for broken references.
Tue, Apr 30, 4:03 AM
grimar committed rL359552: [llvm-objcopy] - Check dynamic relocation sections for broken references..
[llvm-objcopy] - Check dynamic relocation sections for broken references.
Tue, Apr 30, 4:00 AM
grimar closed D60825: [llvm-objcopy] - Check dynamic relocation sections for broken references..
Tue, Apr 30, 4:00 AM · Restricted Project
grimar planned changes to D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol..

Ok. Planning changes. Will take some time since I will be off starting from tomorrow by EOW because of holidays here.

Tue, Apr 30, 3:43 AM · Restricted Project
grimar added inline comments to D61201: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic).
Tue, Apr 30, 3:34 AM
grimar added a comment to D61201: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic).

Generally looks good I think. Few more comments/suggestions from me.

Tue, Apr 30, 3:27 AM
grimar accepted D61298: [LLD] Emit dynamic relocations for references to script symbols in -pie links.

LGTM too (with 2 nits).

Tue, Apr 30, 3:04 AM · Restricted Project
grimar added a comment to D61197: [LLD][ELF] Fix getRankProximity to "ignore" not live sections.

@MaskRay is right that the linker script can be simplified to SECTIONS { .pad : { QUAD(0); } .text : { *(.text) } .ro : { *(.ro) } }, I think it looks much simpler.

Tue, Apr 30, 2:45 AM · Restricted Project
grimar added a comment to D61197: [LLD][ELF] Fix getRankProximity to "ignore" not live sections.

@MaskRay is right that the linker script can be simplified to SECTIONS { .pad : { QUAD(0); } .text : { *(.text) } .ro : { *(.ro) } }, I think it looks much simpler.

Tue, Apr 30, 1:47 AM · Restricted Project

Mon, Apr 29

grimar added a comment to D61180: [yaml2obj] - Allow setting st_value explicitly for Symbol..

I have a slight concern here, in that the approach prevents us using a name that is actually a number, and that might be confusing. I think we might want to have a different field (e.g. "NameIndex" or similar) explicitly for this situation. One and only one of the two fields can then be specified.

What do you think?

Mon, Apr 29, 8:58 AM · Restricted Project
grimar added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).

I agree that it would be nice to ensure that '/DISCARD/' it not output, but the focus of this patch is to fix the undesirable side-effects of the current handling of '/DISCARD/'.

Mon, Apr 29, 7:00 AM · Restricted Project
grimar added a comment to D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).
So I would be happy to hear from other people about the current approach.

Mon, Apr 29, 6:35 AM · Restricted Project
grimar updated subscribers of D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans.
Mon, Apr 29, 6:35 AM · Restricted Project