Page MenuHomePhabricator

grimar (George Rimar)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 21 2015, 12:36 AM (182 w, 1 d)

Recent Activity

Yesterday

grimar added a comment to D59488: [llvm-objcopy] - Calculate the string table section sizes correctly..

LGTM, with a couple of comment nits.

I think you might have flushed out another bug with string table building, based on the size changes in the two archive tests. Could you double-check, and fix or file a bug, please? I'm guessing it's just that we're adding an extra null byte.

Yes, sure, I am actually looking at this :) It seems the issue is that we add a zero's symbol name here (instead of ignoring that symbol):

(https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L498)

// Add all of our strings to SymbolNames so that SymbolNames has the right
// size before layout is decided.
for (auto &Sym : Symbols)
  SymbolNames->addString(Sym->Name);

I am not sure yet, just made this observation during developing this patch.

Mon, Mar 18, 9:42 AM · Restricted Project
grimar created D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry..
Mon, Mar 18, 9:40 AM
grimar committed rGfaf308b11a9f: [llvm-objcopy] - Calculate the string table section sizes correctly. (authored by grimar).
[llvm-objcopy] - Calculate the string table section sizes correctly.
Mon, Mar 18, 7:28 AM
grimar committed rL356371: [llvm-objcopy] - Calculate the string table section sizes correctly..
[llvm-objcopy] - Calculate the string table section sizes correctly.
Mon, Mar 18, 7:26 AM
grimar closed D59488: [llvm-objcopy] - Calculate the string table section sizes correctly..
Mon, Mar 18, 7:26 AM · Restricted Project
grimar added a comment to D59488: [llvm-objcopy] - Calculate the string table section sizes correctly..

LGTM, with a couple of comment nits.

I think you might have flushed out another bug with string table building, based on the size changes in the two archive tests. Could you double-check, and fix or file a bug, please? I'm guessing it's just that we're adding an extra null byte.

Mon, Mar 18, 6:48 AM · Restricted Project
grimar created D59488: [llvm-objcopy] - Calculate the string table section sizes correctly..
Mon, Mar 18, 6:23 AM · Restricted Project

Sun, Mar 17

grimar added a comment to D59333: ELF: Rename Configuration::Static to Bstatic. NFCI..

Maybe we should also update the helptext of Bstatic? It currently says that "Do not link against shared libraries" which doesn't sound like it captures the exact meaning of the option.

"Do not search for dynamic libraries when linking"?

Is it worth mentioning the positional behavior? "Do not search for shared libraries mentioned on the command line after the option"

Sun, Mar 17, 10:06 AM · Restricted Project
grimar committed rG738146ab333e: [LLD][ELF] - Replace one of the tests with a YAML version. (authored by grimar).
[LLD][ELF] - Replace one of the tests with a YAML version.
Sun, Mar 17, 8:45 AM
grimar committed rL356334: [LLD][ELF] - Replace one of the tests with a YAML version..
[LLD][ELF] - Replace one of the tests with a YAML version.
Sun, Mar 17, 8:45 AM
grimar committed rLLD356334: [LLD][ELF] - Replace one of the tests with a YAML version..
[LLD][ELF] - Replace one of the tests with a YAML version.
Sun, Mar 17, 8:45 AM
grimar closed D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
Sun, Mar 17, 8:44 AM · Restricted Project
grimar added a comment to D59333: ELF: Rename Configuration::Static to Bstatic. NFCI..

Maybe we should also update the helptext of Bstatic? It currently says that "Do not link against shared libraries" which doesn't sound like it captures the exact meaning of the option.

Sun, Mar 17, 7:34 AM · Restricted Project

Fri, Mar 15

grimar added inline comments to D59372: [yaml2obj]Allow explicit setting of p_filesz, p_memsz, and p_offset.
Fri, Mar 15, 3:55 AM · Restricted Project
grimar added a comment to D59372: [yaml2obj]Allow explicit setting of p_filesz, p_memsz, and p_offset.

I have a minor question. I do not think it is really important so should not be stoppable for landing this.

Fri, Mar 15, 3:26 AM · Restricted Project
grimar added a comment to D59033: [llvm-objcopy] Make .build-id linking atomic.

I just do not feel I am a good person to review/accept that, overall looks OK for my eye though,
a minor nit/question/suggestion is inline.

Fri, Mar 15, 3:02 AM · Restricted Project
grimar added inline comments to D59269: ELF: Use bump pointer allocator for uncompressed section buffers. NFCI..
Fri, Mar 15, 2:21 AM · Restricted Project

Wed, Mar 13

grimar accepted D59293: [llvm-objcopy]Don't implicitly strip sections in segments.

I approve, but please wait for somebody's else opinion too.

Wed, Mar 13, 5:38 AM · Restricted Project
grimar added inline comments to D59269: ELF: Use bump pointer allocator for uncompressed section buffers. NFCI..
Wed, Mar 13, 1:25 AM · Restricted Project

Tue, Mar 12

grimar added a comment to D59216: [LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection.

Just a few nits about coding style from me.

Tue, Mar 12, 8:31 AM
grimar added a comment to D59186: [llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`.

I am just kidding. Waiting for BoF..

Tue, Mar 12, 5:47 AM · Restricted Project
grimar added a comment to D59186: [llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`.

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019.

Cool! I would like to watch it on Youtube ...
Thank you for taking time review this.

BoFs are not usually recorded

Tue, Mar 12, 5:46 AM · Restricted Project
grimar accepted D59240: ELF: Don't add .dynamic strings to .dynstr early..

I think I have no objections, but please wait for Rui accept too.

Tue, Mar 12, 4:51 AM · Restricted Project
grimar added a comment to D59186: [llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`.

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019. I think
I need to listen to it to understand the desired direction of tools.

Tue, Mar 12, 4:45 AM · Restricted Project
grimar committed rLLD355909: [LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages..
[LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages.
Tue, Mar 12, 4:12 AM
grimar committed rG43b6689e6417: [LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages. (authored by grimar).
[LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages.
Tue, Mar 12, 4:12 AM
grimar committed rL355909: [LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages..
[LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages.
Tue, Mar 12, 4:11 AM
grimar closed D58577: [LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages..
Tue, Mar 12, 4:11 AM · Restricted Project
grimar added inline comments to D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
Tue, Mar 12, 2:23 AM · Restricted Project
grimar updated the diff for D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
  • Addressed review comments.
Tue, Mar 12, 2:23 AM · Restricted Project
grimar accepted D59239: ELF: Simplify. NFCI..

LGTM

Tue, Mar 12, 2:00 AM · Restricted Project

Mon, Mar 11

grimar added a comment to rL355748: [LLD] Fixed flaky unit test based on build directory..

LGTM

Mon, Mar 11, 9:25 AM
grimar committed rG33e498b785cf: [yaml2obj] - Simplify. NFC. (authored by grimar).
[yaml2obj] - Simplify. NFC.
Mon, Mar 11, 9:09 AM
grimar added inline comments to D59097: [yaml2obj]Lookup relocation symbols in dynamic symbol when .dynsym referenced.
Mon, Mar 11, 9:09 AM · Restricted Project
grimar committed rL355832: [yaml2obj] - Simplify. NFC..
[yaml2obj] - Simplify. NFC.
Mon, Mar 11, 9:09 AM
grimar added inline comments to D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
Mon, Mar 11, 9:04 AM · Restricted Project
grimar added a comment to D59085: [LLD][ELF] - Replace one of the tests with a YAML version..

Could you look, Rui? This one is trivial, and I posted this because of D59082, which was accepted.

Mon, Mar 11, 8:45 AM · Restricted Project
grimar committed rGd8a5c6cf19e7: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations. (authored by grimar).
[llvm-objcopy] - Fix --compress-debug-sections when there are relocations.
Mon, Mar 11, 4:01 AM
grimar committed rL355821: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
[llvm-objcopy] - Fix --compress-debug-sections when there are relocations.
Mon, Mar 11, 4:00 AM
grimar closed D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
Mon, Mar 11, 4:00 AM · Restricted Project
grimar added inline comments to D59175: [llvm-readobj] Print symbol version when dumping relocations (PR31564).
Mon, Mar 11, 3:44 AM · Restricted Project
grimar added a comment to D59170: [lld] [ELF] Print a better error for an archive containing a non-ELF file..

I am not fond of adding the precompiled binaries to inputs.
Is it possible to use echo to create such an archive?
Or maybe you could use llvm-ar + empty file maybe (or something like that)?

Mon, Mar 11, 3:16 AM · Restricted Project
grimar added a comment to rL355748: [LLD] Fixed flaky unit test based on build directory..

@grimar - Just FYI. If you know of any other tests that are possibly flaky like this, would be great to patch up :) Thanks!

Mon, Mar 11, 2:22 AM
grimar updated subscribers of rL355748: [LLD] Fixed flaky unit test based on build directory..
Mon, Mar 11, 2:22 AM
grimar added a comment to D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..

It's probably worth adding a TODO or filing an enhancement bug for the other section types this makes sense for, e.g. symbol tables (to move symbols to the new section) or group sections (update referred-to symbol table/change group contents etc).

Mon, Mar 11, 2:13 AM · Restricted Project
grimar accepted D59097: [yaml2obj]Lookup relocation symbols in dynamic symbol when .dynsym referenced.

LGTM.

Mon, Mar 11, 1:14 AM · Restricted Project

Thu, Mar 7

grimar committed rGa5a0a0f0493c: [yaml2obj] - Allow producing ELFDATANONE ELFs (authored by grimar).
[yaml2obj] - Allow producing ELFDATANONE ELFs
Thu, Mar 7, 4:10 AM
grimar committed rL355591: [yaml2obj] - Allow producing ELFDATANONE ELFs.
[yaml2obj] - Allow producing ELFDATANONE ELFs
Thu, Mar 7, 4:10 AM
grimar closed D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.
Thu, Mar 7, 4:10 AM · Restricted Project
grimar added inline comments to D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.
Thu, Mar 7, 3:47 AM · Restricted Project
grimar updated the diff for D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.
  • Addressed review comments.
Thu, Mar 7, 3:47 AM · Restricted Project
grimar added a comment to D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.

What about support for other invalid data encodings e.g. 3, 4 etc?

Thu, Mar 7, 3:25 AM · Restricted Project
grimar updated the diff for D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
  • Added missing "# REQUIRES: x86"
Thu, Mar 7, 3:00 AM · Restricted Project
grimar added a parent revision for D59085: [LLD][ELF] - Replace one of the tests with a YAML version.: D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.
Thu, Mar 7, 2:58 AM · Restricted Project
grimar added a child revision for D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs: D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
Thu, Mar 7, 2:58 AM · Restricted Project
grimar created D59085: [LLD][ELF] - Replace one of the tests with a YAML version..
Thu, Mar 7, 2:58 AM · Restricted Project
grimar requested review of D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.

This is fine as is. Just LLD side requires a slightly more complex test than I expected initially.
It is: https://reviews.llvm.org/D59085

Thu, Mar 7, 2:58 AM · Restricted Project
grimar planned changes to D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.

That is not enough it seems. Planning change.

Thu, Mar 7, 2:34 AM · Restricted Project
grimar created D59082: [yaml2obj] - Allow producing ELFDATANONE ELFs.
Thu, Mar 7, 2:10 AM · Restricted Project
grimar accepted D59081: [llvm-objcopy] Remove unneeded checks. NFC.

LGTM

Thu, Mar 7, 2:03 AM · Restricted Project
grimar accepted D59044: ELF: Reduce the size of InputSectionBase by two words. NFCI..

LGTM

Thu, Mar 7, 12:35 AM · Restricted Project

Wed, Mar 6

grimar committed rG281a5beefa81: [llvm-objcopy] - Remove dead code. NFCI. (authored by grimar).
[llvm-objcopy] - Remove dead code. NFCI.
Wed, Mar 6, 6:12 AM
grimar committed rL355505: [llvm-objcopy] - Remove dead code. NFCI..
[llvm-objcopy] - Remove dead code. NFCI.
Wed, Mar 6, 6:11 AM
grimar closed D59017: [llvm-objcopy] - Remove dead code. NFCI..
Wed, Mar 6, 6:11 AM · Restricted Project
grimar committed rG5f0b7d2f4688: [llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code. (authored by grimar).
[llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code.
Wed, Mar 6, 6:09 AM
grimar committed rL355503: [llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code..
[llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code.
Wed, Mar 6, 6:09 AM
grimar closed D59019: [llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code..
Wed, Mar 6, 6:09 AM · Restricted Project
grimar committed rGf2eb8caa3fbb: [llvm-objcopy] - Fix incorrect CompressedSection creation. (authored by grimar).
[llvm-objcopy] - Fix incorrect CompressedSection creation.
Wed, Mar 6, 6:03 AM
grimar committed rL355501: [llvm-objcopy] - Fix incorrect CompressedSection creation..
[llvm-objcopy] - Fix incorrect CompressedSection creation.
Wed, Mar 6, 6:02 AM
grimar closed D59018: [llvm-objcopy] - Fix incorrect CompressedSection creation..
Wed, Mar 6, 6:02 AM · Restricted Project
grimar committed rGa033572d67bf: [LLD][ELF] - Convert common-symbol-alignment.s test to yaml. (authored by grimar).
[LLD][ELF] - Convert common-symbol-alignment.s test to yaml.
Wed, Mar 6, 5:50 AM
grimar committed rLLD355500: [LLD][ELF] - Convert common-symbol-alignment.s test to yaml..
[LLD][ELF] - Convert common-symbol-alignment.s test to yaml.
Wed, Mar 6, 5:49 AM
grimar committed rL355500: [LLD][ELF] - Convert common-symbol-alignment.s test to yaml..
[LLD][ELF] - Convert common-symbol-alignment.s test to yaml.
Wed, Mar 6, 5:49 AM
grimar updated the diff for D58577: [LLD][ELF] - Show symbols visibility in "undefined symbol..." error messages..

Are we going to do this change?

Wed, Mar 6, 5:07 AM · Restricted Project
grimar added a comment to D59018: [llvm-objcopy] - Fix incorrect CompressedSection creation..

The code change looks good, but I'm not sure I understand why you can't test this, i.e. where you have an input with a compressed section starting with .zdebug and showing that it is decompressed?

Wed, Mar 6, 4:56 AM · Restricted Project
grimar added inline comments to D59019: [llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code..
Wed, Mar 6, 4:46 AM · Restricted Project
grimar added inline comments to D59017: [llvm-objcopy] - Remove dead code. NFCI..
Wed, Mar 6, 4:42 AM · Restricted Project
grimar committed rGd024a9fab3c6: [LLD][ELF] - Remove unused invalid input object. NFC. (authored by grimar).
[LLD][ELF] - Remove unused invalid input object. NFC.
Wed, Mar 6, 4:37 AM
grimar committed rL355497: [LLD][ELF] - Remove unused invalid input object. NFC..
[LLD][ELF] - Remove unused invalid input object. NFC.
Wed, Mar 6, 4:37 AM
grimar committed rLLD355497: [LLD][ELF] - Remove unused invalid input object. NFC..
[LLD][ELF] - Remove unused invalid input object. NFC.
Wed, Mar 6, 4:37 AM
grimar created D59019: [llvm-objcopy] - Remove an excessive zlib::isAvailable() check and dead code..
Wed, Mar 6, 3:56 AM · Restricted Project
grimar retitled D59018: [llvm-objcopy] - Fix incorrect CompressedSection creation. from [llvm-objdump] - Fix incorrect CompressedSection creation. to [llvm-objcopy] - Fix incorrect CompressedSection creation..
Wed, Mar 6, 3:32 AM · Restricted Project
grimar updated the summary of D59018: [llvm-objcopy] - Fix incorrect CompressedSection creation..
Wed, Mar 6, 3:07 AM · Restricted Project
grimar created D59018: [llvm-objcopy] - Fix incorrect CompressedSection creation..
Wed, Mar 6, 3:07 AM · Restricted Project
grimar created D59017: [llvm-objcopy] - Remove dead code. NFCI..
Wed, Mar 6, 2:48 AM · Restricted Project

Tue, Mar 5

grimar added inline comments to D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
Tue, Mar 5, 7:04 AM · Restricted Project
grimar added inline comments to D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
Tue, Mar 5, 6:01 AM · Restricted Project
grimar updated the diff for D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
  • Addressed review comments.
Tue, Mar 5, 6:01 AM · Restricted Project
grimar committed rGade3c70537e1: [llvm-objcopy] - Simplify `isCompressable` and fix the issue relative. (authored by grimar).
[llvm-objcopy] - Simplify `isCompressable` and fix the issue relative.
Tue, Mar 5, 5:08 AM
grimar committed rL355399: [llvm-objcopy] - Simplify `isCompressable` and fix the issue relative..
[llvm-objcopy] - Simplify `isCompressable` and fix the issue relative.
Tue, Mar 5, 5:07 AM
grimar closed D58908: [llvm-objcopy] - SImplify `isCompressable` and fix the issue relative..
Tue, Mar 5, 5:07 AM · Restricted Project
grimar created D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
Tue, Mar 5, 4:34 AM · Restricted Project
grimar added inline comments to D58908: [llvm-objcopy] - SImplify `isCompressable` and fix the issue relative..
Tue, Mar 5, 3:48 AM · Restricted Project
grimar updated the diff for D58908: [llvm-objcopy] - SImplify `isCompressable` and fix the issue relative..
  • Addressed review comments.
Tue, Mar 5, 3:48 AM · Restricted Project
grimar committed rG1e93080ca854: [llvm-objcopy] - Report "no zlib available" error properly when --compress… (authored by grimar).
[llvm-objcopy] - Report "no zlib available" error properly when --compress…
Tue, Mar 5, 3:33 AM
grimar committed rL355391: [llvm-objcopy] - Report "no zlib available" error properly when --compress….
[llvm-objcopy] - Report "no zlib available" error properly when --compress…
Tue, Mar 5, 3:33 AM
grimar closed D58909: [llvm-objcopy] - Report "no zlib available" error properly when --compress-debug-sections is used..
Tue, Mar 5, 3:33 AM · Restricted Project
grimar updated the summary of D58909: [llvm-objcopy] - Report "no zlib available" error properly when --compress-debug-sections is used..
Tue, Mar 5, 2:43 AM · Restricted Project
grimar added a comment to D58909: [llvm-objcopy] - Report "no zlib available" error properly when --compress-debug-sections is used..

By the way, is this fixing https://bugs.llvm.org/show_bug.cgi?id=40886? I think it does for me.

Tue, Mar 5, 2:40 AM · Restricted Project
grimar added a comment to D58909: [llvm-objcopy] - Report "no zlib available" error properly when --compress-debug-sections is used..

Can you write a test that is UNSUPPORTED if zlib IS available?

I think we have such a test already. It is:
https://github.com/llvm-mirror/llvm/blob/master/test/tools/llvm-objcopy/ELF/compress-debug-sections-invalid-format.test
(It does not do # REQUIRES: zlib because code report the Invalid or unsupported --compress-debug-sections format error before zlib::isAvailable() check.
What looks fine to me.)

That isn't the same test as I meant. That checks the error message at line 475, whereas I'm looking for a test for line 483. In other words, a valid compression format, but where zlib::isAvailable fails. It may not be possible of course.

Tue, Mar 5, 2:05 AM · Restricted Project