Page MenuHomePhabricator

grimar (George Rimar)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 21 2015, 12:36 AM (279 w, 2 d)

Recent Activity

Today

grimar added a comment to D95341: [yaml2obj] - Allow empty SectionHeaderTable definitions..

LGTM.

Hi guys.
I am sorry for pinging this too early, but I am planning a ~6 weeks vacation starting from 1st of february
and I wonder if there is something I can do for this to land it or if there are any concerns about this change?

No problem. Somehow this slipped off my review pile, so I hadn't seen it. Let me know if there are any other reviews I haven't done by the end of the day.

Wed, Jan 27, 1:50 AM · Restricted Project
grimar added inline comments to D95461: [llvm-nm] Display defined weak STT_GNU_IFUNC symbols as 'i'.
Wed, Jan 27, 12:22 AM · Restricted Project

Yesterday

grimar added a comment to D95341: [yaml2obj] - Allow empty SectionHeaderTable definitions..

Hi guys.
I am sorry for pinging this too early, but I am planning a ~6 weeks vacation starting from 1st of february
and I wonder if there is something I can do for this to land it or if there are any concerns about this change?

Tue, Jan 26, 11:48 PM · Restricted Project
grimar committed rGd5e48f1347d5: [yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field. (authored by grimar).
[yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field.
Tue, Jan 26, 2:33 AM
grimar closed D95364: [yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field..
Tue, Jan 26, 2:33 AM · Restricted Project
grimar added inline comments to D67137: [llvm-objcopy] Implement --only-keep-debug for ELF.
Tue, Jan 26, 2:10 AM · Restricted Project
grimar added a comment to D95354: [yaml2obj] - Refine how we set the sh_link field. NFCI..

It turns out this wasn't NFC, one of LLDB test cases started to crash.
I've fixed it in rG2a33b092f5b175a21680b3980ff2019bc1c65b12.

The difference was that before this patch we set the sh_link for symbol tables basing on their names (E.g ".symtab"),
now we set this field basing on section types (e.g SHT_SYMTAB).
The mentioned test case has a .text section of type SHT_SYMTAB and it did not expect to see a sh_link field set it seeems.

Based on my casual reading of that test, I wonder if you found a bug in the tool there - it looks like the intent is for section types to trump section names, which surely should mean the use of sh_link would be correctin this case? Perhaps we should highlight this to the LLDB developers (pinging @JDevlieghere for example).

Tue, Jan 26, 2:04 AM · Restricted Project
grimar added a comment to D95364: [yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field..

E.g. it seems that the expected behavior of llvm-objcopy would be to drop the sh_link for symbol table in this case. Because it is of SHT_NOBITS type.
Perhaps I'd wait from some input from @MaskRay as he was the author of the test case I think.

Tue, Jan 26, 1:59 AM · Restricted Project
grimar added a comment to D95364: [yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field..

Looks reasonable, but perhaps fixing the llvm-objcopy issue first would be best. What do you think?

Tue, Jan 26, 1:50 AM · Restricted Project
grimar added a comment to D95354: [yaml2obj] - Refine how we set the sh_link field. NFCI..

It turns out this wasn't NFC, one of LLDB test cases started to crash.
I've fixed it in rG2a33b092f5b175a21680b3980ff2019bc1c65b12.

Tue, Jan 26, 1:48 AM · Restricted Project
grimar committed rG2a33b092f5b1: [LLDB][test] - Fix test after yaml2obj change. (authored by grimar).
[LLDB][test] - Fix test after yaml2obj change.
Tue, Jan 26, 1:43 AM
grimar committed rG029644ee5107: [yaml2obj] - Refine how we set the sh_link field. NFCI. (authored by grimar).
[yaml2obj] - Refine how we set the sh_link field. NFCI.
Tue, Jan 26, 1:28 AM
grimar closed D95354: [yaml2obj] - Refine how we set the sh_link field. NFCI..
Tue, Jan 26, 1:28 AM · Restricted Project
grimar committed rGdb92d47cf70e: [llvm-nm][ELF] - Use @@ prefix when printing default versions. (authored by grimar).
[llvm-nm][ELF] - Use @@ prefix when printing default versions.
Tue, Jan 26, 1:17 AM
grimar closed D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Tue, Jan 26, 1:17 AM · Restricted Project
grimar added inline comments to D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Tue, Jan 26, 1:16 AM · Restricted Project
grimar committed rGe98d5c31925d: [libObject,llvm-readelf/obj] - Don't use @@ when printing versions of undefined… (authored by grimar).
[libObject,llvm-readelf/obj] - Don't use @@ when printing versions of undefined…
Tue, Jan 26, 1:06 AM
grimar closed D95219: [libObject,llvm-readelf/obj] - Don't use @@ when printing versions of undefined symbols..
Tue, Jan 26, 1:06 AM · Restricted Project
grimar added a comment to D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued.

I like this approach.

Tue, Jan 26, 12:24 AM · Restricted Project, Restricted Project

Mon, Jan 25

grimar requested review of D95364: [yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field..
Mon, Jan 25, 7:28 AM · Restricted Project
grimar requested review of D95354: [yaml2obj] - Refine how we set the sh_link field. NFCI..
Mon, Jan 25, 5:56 AM · Restricted Project
grimar requested review of D95341: [yaml2obj] - Allow empty SectionHeaderTable definitions..
Mon, Jan 25, 3:47 AM · Restricted Project
grimar committed rG19245b781576: [ObjectYAML] - An attempt to fix BB after commit of D95140. (authored by grimar).
[ObjectYAML] - An attempt to fix BB after commit of D95140.
Mon, Jan 25, 2:26 AM
grimar committed rG9c89dcf80736: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk. (authored by grimar).
[yaml2obj, obj2yaml] - Implement section header table as a special Chunk.
Mon, Jan 25, 2:09 AM
grimar closed D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk..
Mon, Jan 25, 2:08 AM · Restricted Project
grimar updated the diff for D95219: [libObject,llvm-readelf/obj] - Don't use @@ when printing versions of undefined symbols..
  • Addressed review comment.
Mon, Jan 25, 1:52 AM · Restricted Project
grimar added a comment to D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued.

As far I understand, looking on the description of D94239, the message on z/OS looks like "EDC5129I No such file or directory.".
I guess the EDC5129I is a stable error code? So why not to check for a possible EDC5129I prefix word instead of .*?

Mon, Jan 25, 1:32 AM · Restricted Project, Restricted Project
grimar added a comment to D95158: Use DataExtractor to decode SLEB128 in android_relas..

A minor nit from me, the rest looks fine.

Mon, Jan 25, 1:23 AM · Restricted Project
grimar added a reviewer for D95199: [ELF] Write output sections in PT_LOAD segment order: psmith.
Mon, Jan 25, 1:05 AM · Restricted Project
grimar added a comment to D95199: [ELF] Write output sections in PT_LOAD segment order.

This looks to me it solves a specific corner case, but I really don't feel I have enough knowledges about embedded world/scripts to say whether it is a good or bad thing to support it.
It seems matches to what GNU ld/gold do. I'll add Peter who I guess might have an opinion about this.

Mon, Jan 25, 1:05 AM · Restricted Project

Fri, Jan 22

grimar added inline comments to D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk..
Fri, Jan 22, 5:30 AM · Restricted Project
grimar updated the diff for D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk..
  • Addressed review comments.
Fri, Jan 22, 5:30 AM · Restricted Project
grimar abandoned D93678: [yaml2obj] - Support selecting the location of the section header table..

D95140 was posted instead.

Fri, Jan 22, 4:53 AM · Restricted Project
grimar updated the diff for D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
  • Addressed review comments.
Fri, Jan 22, 4:50 AM · Restricted Project
grimar added inline comments to D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Fri, Jan 22, 3:57 AM · Restricted Project
grimar added inline comments to D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Fri, Jan 22, 3:56 AM · Restricted Project
grimar requested review of D95219: [libObject,llvm-readelf/obj] - Don't use @@ when printing versions of undefined symbols..
Fri, Jan 22, 3:28 AM · Restricted Project
grimar added a comment to D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk..

It feels like there needs to be testing there for writing the section header table in the right place in the YAML (with Offset if needed), but I don't see that yet.

Fri, Jan 22, 1:46 AM · Restricted Project

Thu, Jan 21

grimar added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Thu, Jan 21, 11:13 PM · Restricted Project
grimar requested review of D95140: [yaml2obj, obj2yaml] - Implement section header table as a special Chunk..
Thu, Jan 21, 7:45 AM · Restricted Project
grimar accepted D95124: [lld][ELF][test] Add testing for IE/LD TLS weak undef references.

LGTM with a nit, please wait for others.

Thu, Jan 21, 4:54 AM · Restricted Project
grimar added a comment to D95124: [lld][ELF][test] Add testing for IE/LD TLS weak undef references.

missing check-prefix. Not sure how I didn't spot the latter...

Thu, Jan 21, 4:42 AM · Restricted Project
grimar added inline comments to D95124: [lld][ELF][test] Add testing for IE/LD TLS weak undef references.
Thu, Jan 21, 4:16 AM · Restricted Project
grimar committed rGdd5c98280473: [llvm-nm][ELF] - Make -D display symbol versions. (authored by grimar).
[llvm-nm][ELF] - Make -D display symbol versions.
Thu, Jan 21, 12:25 AM
grimar closed D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Thu, Jan 21, 12:24 AM · Restricted Project

Wed, Jan 20

grimar committed rG51f4958057d6: [yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections. (authored by grimar).
[yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections.
Wed, Jan 20, 11:52 PM
grimar closed D94956: [yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections..
Wed, Jan 20, 11:51 PM · Restricted Project
grimar accepted D94560: [ELF] report section sizes when output file too large.

This LGTM, but please wait for others.

Wed, Jan 20, 11:29 PM · Restricted Project
grimar added a comment to D94907: [llvm-nm][ELF] - Make -D display symbol versions..

@MaskRay, are you OK with it?

Wed, Jan 20, 12:32 AM · Restricted Project

Tue, Jan 19

grimar added a comment to D93761: [libObject/Decompressor] - Use `resize_for_overwrite` in Decompressor::resizeAndDecompress()..

Ping.

Tue, Jan 19, 7:05 AM · Restricted Project
grimar updated the diff for D94956: [yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections..
  • Added test cases.
Tue, Jan 19, 7:05 AM · Restricted Project
grimar added inline comments to D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Tue, Jan 19, 6:11 AM · Restricted Project
grimar updated the diff for D94907: [llvm-nm][ELF] - Make -D display symbol versions..
  • Addressed review comment.
Tue, Jan 19, 6:06 AM · Restricted Project
grimar added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Tue, Jan 19, 5:51 AM · Restricted Project
grimar added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Tue, Jan 19, 2:26 AM · Restricted Project
grimar updated the diff for D94907: [llvm-nm][ELF] - Make -D display symbol versions..
  • Addressed review comments.
Tue, Jan 19, 2:26 AM · Restricted Project
grimar requested review of D94956: [yaml2obj/obj2yaml] - Improve dumping/creating of ELF versioning sections..
Tue, Jan 19, 2:05 AM · Restricted Project

Mon, Jan 18

grimar requested review of D94912: [llvm-nm][ELF] - Use @@ prefix when printing default versions..
Mon, Jan 18, 5:44 AM · Restricted Project
grimar updated the diff for D94907: [llvm-nm][ELF] - Make -D display symbol versions..
  • Format.
Mon, Jan 18, 4:33 AM · Restricted Project
grimar added inline comments to D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Mon, Jan 18, 4:31 AM · Restricted Project
grimar requested review of D94907: [llvm-nm][ELF] - Make -D display symbol versions..
Mon, Jan 18, 4:29 AM · Restricted Project
grimar committed rGb9ce772b8fb5: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h (authored by grimar).
[Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h
Mon, Jan 18, 1:51 AM
grimar closed D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.
Mon, Jan 18, 1:50 AM · Restricted Project
grimar updated the diff for D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.
  • Addressed review comments.
Mon, Jan 18, 12:58 AM · Restricted Project
grimar updated the summary of D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.
Mon, Jan 18, 12:54 AM · Restricted Project
grimar added inline comments to D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.
Mon, Jan 18, 12:49 AM · Restricted Project
grimar added a comment to D94560: [ELF] report section sizes when output file too large.

Thanks again.

I chose DAG on purpose because I think it more accurately expresses what we're really asserting.

First, we since we removed the sorting, we're not guaranteeing that these will be in any particular order. Given that, I would rather not make the test depend on the order.

Mon, Jan 18, 12:22 AM · Restricted Project

Fri, Jan 15

grimar requested review of D94771: [Object, llvm-readelf] - Move the API for retrieving symbol versions to ELF.h.
Fri, Jan 15, 5:48 AM · Restricted Project
grimar committed rG45ef053bd709: [llvm-readobj][test] - Remove excessive YAML fields from tests. (authored by grimar).
[llvm-readobj][test] - Remove excessive YAML fields from tests.
Fri, Jan 15, 1:53 AM
grimar closed D94660: [llvm-readobj][test] - Remove excessive YAML fields from tests..
Fri, Jan 15, 1:53 AM · Restricted Project
grimar committed rGd9afe8588e49: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections. (authored by grimar).
[yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections.
Fri, Jan 15, 1:42 AM
grimar closed D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
Fri, Jan 15, 1:42 AM · Restricted Project
grimar committed rG021ea78a97ed: [llvm-nm] - Simplify the code in dumpSymbolNamesFromObject. NFC. (authored by grimar).
[llvm-nm] - Simplify the code in dumpSymbolNamesFromObject. NFC.
Fri, Jan 15, 1:30 AM
grimar closed D94669: [llvm-nm] - Simplify the code in dumpSymbolNamesFromObject. NFC..
Fri, Jan 15, 1:30 AM · Restricted Project
grimar committed rGbfb8f45ef3f4: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). (authored by grimar).
[llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject().
Fri, Jan 15, 1:19 AM
grimar closed D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Fri, Jan 15, 1:19 AM · Restricted Project
grimar committed rG1185d3f43d21: [llvm-readobj] - Fix the compilation with GCC < 7.0. (authored by grimar).
[llvm-readobj] - Fix the compilation with GCC < 7.0.
Fri, Jan 15, 12:59 AM
grimar added a comment to D94560: [ELF] report section sizes when output file too large.

An alternative to this could probably be changing the Writer<ELFT>::assignFileOffsets()
It has the following loop:

Fri, Jan 15, 12:44 AM · Restricted Project
grimar added a comment to D94560: [ELF] report section sizes when output file too large.

Thanks for the comments!

E.g. I think it can be just a 2 lines loop which would iterate over all output sections and print their names ans sizes.
No need to sort them or to calculate fields width.

The reason I do the sorting is because there can potentially be very many sections. Imagine a large binary with -fdata-sections -ffunction-settings.

Fri, Jan 15, 12:29 AM · Restricted Project
grimar added inline comments to D93900: [llvm-readef/obj] - Change the design structure of ELF dumper. NFCI..
Fri, Jan 15, 12:22 AM · Restricted Project
grimar updated the summary of D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
Fri, Jan 15, 12:08 AM · Restricted Project
grimar added a comment to D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..

But I think using 0 in obj2yaml is fine.

Fri, Jan 15, 12:07 AM · Restricted Project

Thu, Jan 14

grimar added inline comments to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.
Thu, Jan 14, 11:51 PM · Restricted Project
grimar added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

For the case to test error message when sh_size % sh_entsize is not 0, I didn't find any good examples that I can manipulate the sh_entsize through yaml2obj. Do you have good suggestions to write unit tests for these cases?

Thu, Jan 14, 11:46 PM · Restricted Project
grimar added a comment to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.

The reason is that .strtab and .shstrtab are placed at the end of elf file automatically by yaml2obj and there will almost always be cases to make (*It) & 1 != 0, preventing the code from reach the error message. I think the only way would be placing .gnu.hash at the end of the ELF, and remove the terminator, but that seems to be not possible with yaml2obj. For the case to test error message when sh_size % sh_entsize is not 0, I didn't find any good examples that I can manipulate the sh_entsize through yaml2obj. Do you have good suggestions to write unit tests for these cases?

Thu, Jan 14, 11:37 PM · Restricted Project
grimar added a comment to D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..

Does it have to stay in llvm-nm.cpp? A different file for MachO stuff?

Thu, Jan 14, 5:31 AM · Restricted Project
grimar added inline comments to D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Thu, Jan 14, 4:28 AM · Restricted Project
grimar added inline comments to D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Thu, Jan 14, 4:27 AM · Restricted Project
grimar added inline comments to D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
Thu, Jan 14, 4:15 AM · Restricted Project
grimar updated the diff for D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
  • Addressed review comments.
Thu, Jan 14, 4:15 AM · Restricted Project
grimar requested review of D94669: [llvm-nm] - Simplify the code in dumpSymbolNamesFromObject. NFC..
Thu, Jan 14, 3:52 AM · Restricted Project
grimar updated the summary of D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Thu, Jan 14, 3:36 AM · Restricted Project
grimar requested review of D94667: [llvm-nm] - Move MachO specific logic out from the dumpSymbolNamesFromObject(). NFC..
Thu, Jan 14, 3:35 AM · Restricted Project
grimar requested review of D94660: [llvm-readobj][test] - Remove excessive YAML fields from tests..
Thu, Jan 14, 1:36 AM · Restricted Project
grimar requested review of D94659: [yaml2obj/obj2yaml] - Refine handling of SHT_GNU_verdef sections..
Thu, Jan 14, 1:31 AM · Restricted Project
grimar added a comment to D94560: [ELF] report section sizes when output file too large.

FWIW, I think we could support this, but I'd implement it in a much more trivial way.
E.g. I think it can be just a 2 lines loop which would iterate over all output sections and print their names ans sizes.
No need to sort them or to calculate fields width.

Thu, Jan 14, 12:05 AM · Restricted Project

Wed, Jan 13

grimar added inline comments to D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section.
Wed, Jan 13, 11:57 PM · Restricted Project
grimar added a comment to D94620: [NFC] Disallow unused prefixes under MC/ARM.

I am not the right person to review this, I don't work on MC or MC/ARM. I think you should add people who mantains MC/ARM and/or may be authors of these tests.
The change looks good to me though.

Wed, Jan 13, 11:41 PM · Restricted Project
grimar added a comment to D93761: [libObject/Decompressor] - Use `resize_for_overwrite` in Decompressor::resizeAndDecompress()..

Ok. I've used the following piece of code to construct a 1Gb of random compressed data and estimated
the difference between the old and the new code in Release.

Wed, Jan 13, 4:11 AM · Restricted Project
grimar added a comment to D93678: [yaml2obj] - Support selecting the location of the section header table..

An alternative would be to move the Offset aspect of the sections into the Layout table only, and possibly moving Fills into that Layout block too out of the Sections list. This has the advantage that layout information is generally kept in the Layout table, whilst the details of what a thing actually is (e.g. section type, size, address etc) are kept in appropriate parts of the document. This is closer to my original option 3:

FileHeader:
  ...
Sections:
  - Name: .foo
    Type: SHT_PROGBITS
  - Name: .bar
    Type: SHT_PROGBITS
SectionHeaderTable:
  ...
ProgramHeaders:
  ...
Layout:
   - Type: Section
    Name: .foo
    Offset: 0x1000
  - Type: Fill
    # Offset could be implicitly calculated here; fills move out of Sections to Layout.
    Size: 0x100
  - Type: SectionHeaderTable
    Offset: 0x2000
  - Type: ProgramHeaderTable
    Offset: 0x3000
  - Type: Section
    Name: .bar
    Offset: 0x3500

I think I mildly prefer the second one, as it'll be easier to retrofit and feels a little cleaner. We'd need some implicit rules that the program header table appears at the front and the section header table at the back if they aren't specifically mentioned in either case.

Wed, Jan 13, 1:47 AM · Restricted Project