Page MenuHomePhabricator

jakehehrlich (Jake Ehrlich)
User

Projects

User does not belong to any projects.

User Details

User Since
May 16 2017, 11:34 AM (81 w, 5 d)

Recent Activity

Fri, Dec 7

jakehehrlich accepted D55450: [TextAPI][elfabi] Make TBE handlers functions that return Errors.

Looks like a simple mechanical transformation. LGTM.

Fri, Dec 7, 12:19 PM

Thu, Dec 6

jakehehrlich added inline comments to D55352: [elfabi] Introduce tool for ELF TextAPI.
Thu, Dec 6, 2:20 PM
jakehehrlich added a comment to D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.

Also I'll put up a review for a change that triggers the overflow error and other things. One moment.

Thu, Dec 6, 11:50 AM
jakehehrlich accepted D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration.

Thanks! This looks good to me.

Thu, Dec 6, 11:49 AM

Wed, Dec 5

jakehehrlich committed rL348446: [llvm-objcopy] Change --only-keep to --only-section.
[llvm-objcopy] Change --only-keep to --only-section
Wed, Dec 5, 6:07 PM
jakehehrlich closed D52180: [llvm-objcopy] Change --only-keep to --only-sections.
Wed, Dec 5, 6:07 PM
jakehehrlich updated the diff for D52180: [llvm-objcopy] Change --only-keep to --only-sections.

Rebased and used -only-section (no s at the end)

Wed, Dec 5, 4:02 PM

Mon, Dec 3

jakehehrlich accepted D55244: [compiler-rt] Use the new zx_futex_wait for Fuchsia sanitizer runtime.
Mon, Dec 3, 7:58 PM
jakehehrlich added inline comments to D54939: [RFC] [llvm-objcopy] Initial COFF support.
Mon, Dec 3, 3:11 PM
jakehehrlich added a comment to D54939: [RFC] [llvm-objcopy] Initial COFF support.

Also don't focus on byte for byte accuracy, test semantically so that we can make layout changes independent of content changes. It's clear to me now that we shouldn't even bother attempting exact binary matches even in the first patch.

Mon, Dec 3, 2:46 PM
jakehehrlich added a comment to D54939: [RFC] [llvm-objcopy] Initial COFF support.

On the whole ELFObject vs Object thing. I'm in favor of just using Object but I don't really care too much. Having both the namespace and the name prefix is kind of verbose. A while back we had two kinds of Objects instead of just one but this was determined overtime to be a mistake for the ELF backend. If you want to do conversions between formats, even if the format is "binary" you'll probably want to avoid having two kinds of Object or if you do make sure you provide a sufficiently rich base interface to them to allow most code to be written in terms of that.

Mon, Dec 3, 2:44 PM
jakehehrlich committed rL348174: [llvm-objcopy] Add --build-id-link-dir flag.
[llvm-objcopy] Add --build-id-link-dir flag
Mon, Dec 3, 11:52 AM
jakehehrlich closed D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Mon, Dec 3, 11:52 AM
jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.

Aight, uploading the final patch so people can see exactly what I'm going to push.

Mon, Dec 3, 11:46 AM

Tue, Nov 27

jakehehrlich added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Oh and you'll need to remove the dependency on libObject as discussed. I pointed out that only changes I think you'll need to make but you might find a few more.

Tue, Nov 27, 3:11 PM
jakehehrlich added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

LGTM with nits. Please make sure Jurgen and you agree on the namespace and folder scheme first but other than than this LGTM.

Tue, Nov 27, 3:07 PM
jakehehrlich added a comment to D53945: [TextAPI] TBD Reader/Writer.

Would there be an objection to using llvm::tapi::MachO or llvm::tapi::macho? At least for ELF it doesn't make much sense to put the tapi code in the same namespace as BinaryFormat/ELF so it would be nice to add the extra namespace. I'm not married to the idea however.

Tue, Nov 27, 3:04 PM
jakehehrlich accepted D54936: [llvm-objcopy] Hook up the -V alias to --version, output "GNU strip".

LGTM

Tue, Nov 27, 3:00 PM
jakehehrlich added a comment to D54936: [llvm-objcopy] Hook up the -V alias to --version, output "GNU strip".

I think this is fine. GNU compatibility is a tricky subject with lots of nuances that I keep discovering but it is decidedly a stated goal.

Tue, Nov 27, 3:00 PM
jakehehrlich added a comment to D54674: [llvm-objcopy] First bits for MachO .

I had a few bike-shed level comments. I'll dig into the code more deeply soon but my near complete lack of knowledge of MachO makes giving any actually useful feedback hard.

Tue, Nov 27, 2:55 PM
jakehehrlich added a comment to D54674: [llvm-objcopy] First bits for MachO .

Added Jurgen who should be able to help with the MachO review (or finding a reviewer) as well.

Tue, Nov 27, 1:27 PM
jakehehrlich added a reviewer for D54674: [llvm-objcopy] First bits for MachO : ributzka.
Tue, Nov 27, 1:26 PM
jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Tue, Nov 27, 12:54 PM
jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.

Responded to latest comments.

Tue, Nov 27, 12:54 PM

Mon, Nov 26

jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Mon, Nov 26, 5:22 PM

Mon, Nov 19

jakehehrlich accepted D54741: [Driver] Link sanitizer runtime deps on Fuchsia when needed.
Mon, Nov 19, 11:58 PM
jakehehrlich accepted D54727: [compiler-rt] Use zx_futex_wait_deprecated for Fuchsia sanitizer runtime.
Mon, Nov 19, 4:36 PM
jakehehrlich added a comment to D54674: [llvm-objcopy] First bits for MachO .

Also I'll be gone this week for Thanksgiving. I might look at this off and on but don't expect too much this week

Mon, Nov 19, 1:58 PM
jakehehrlich updated subscribers of D54674: [llvm-objcopy] First bits for MachO .

I like the planned direction for features here! I'm going to add @echristo to this since I think he knows MachO reasonably well and would know someone who can review those aspects for us.

Mon, Nov 19, 1:51 PM

Thu, Nov 15

jakehehrlich accepted D54598: [CMake] Explicitly list Linux targets for Fuchsia toolchain.
Thu, Nov 15, 1:46 PM

Wed, Nov 14

jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Wed, Nov 14, 5:36 PM
jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.
  1. Switched to expected
  2. Used slice
  3. Formatted td file
Wed, Nov 14, 5:36 PM

Tue, Nov 13

jakehehrlich added a comment to D54477: [llvm-objcopy] Rename --keep to --keep-section..

If we wanted we could also add an alias for -k but I don't really think that matters too much.

Tue, Nov 13, 9:32 AM
jakehehrlich accepted D54477: [llvm-objcopy] Rename --keep to --keep-section..

LGTM

Tue, Nov 13, 9:30 AM

Mon, Nov 12

jakehehrlich closed D54451: [libObject] Fix getDesc for Elf_Note_Impl.
Mon, Nov 12, 7:17 PM
jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Mon, Nov 12, 6:37 PM
jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Mon, Nov 12, 6:32 PM
jakehehrlich added a comment to D54384: [llvm-objcopy] Add --build-id-link-dir flag.

Not technically something we can't work around, the following code solves my problem now that I understand it.

Mon, Nov 12, 6:13 PM
jakehehrlich added a comment to D54384: [llvm-objcopy] Add --build-id-link-dir flag.

So I fixed this issue: https://reviews.llvm.org/D54451

Mon, Nov 12, 5:57 PM
jakehehrlich committed rL346724: [libObject] Fix getDesc for Elf_Note_Impl.
[libObject] Fix getDesc for Elf_Note_Impl
Mon, Nov 12, 5:13 PM
jakehehrlich updated the diff for D54451: [libObject] Fix getDesc for Elf_Note_Impl.
  • Added test that would have caused undefined behavior before but should be solid now.
Mon, Nov 12, 4:13 PM
jakehehrlich added a comment to D54451: [libObject] Fix getDesc for Elf_Note_Impl.

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

Mon, Nov 12, 3:42 PM
jakehehrlich created D54451: [libObject] Fix getDesc for Elf_Note_Impl.
Mon, Nov 12, 3:26 PM

Sat, Nov 10

jakehehrlich updated subscribers of D54384: [llvm-objcopy] Add --build-id-link-dir flag.

I started to rewrite it in terms of ELFFile. The main challenge nis dealing
with the Error object that we don't really care about.

Sat, Nov 10, 7:07 PM
jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Sat, Nov 10, 4:25 PM
jakehehrlich created D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Sat, Nov 10, 3:01 PM

Nov 9 2018

jakehehrlich added inline comments to D53051: [llvm-tapi] initial commit, supports ELF text stubs.
Nov 9 2018, 12:00 PM

Nov 6 2018

jakehehrlich updated subscribers of D54193: [llvm-strip] Check "strip" with StringRef::contains instead of ends_with.

I'm not where I can use phab but the diff and reason for the change look
good to me.

Nov 6 2018, 6:24 PM
jakehehrlich added a comment to D53945: [TextAPI] TBD Reader/Writer.

<bikeshed>
I noticed the library is named TextAPI, is there a specific reason for this? My patch names the library TAPI, but if there's a reason not to I can change that.
</bikeshed>

I'd like to start a conversation on where we'd like the ELF and MachO code paths to diverge. It looks like a thin abstraction of InterfaceFile.h that forks into MachO and ELF specific implementations would be reasonable. TextAPIReaderWriter seems generic enough that it should support an ELF implementation without any modification. In the context of this patch, those places look like reasonable points where implementation specifics might diverge. However, depending on what else would be upstreamed following this patch, I'm not sure if that's the best approach.

An alternative option might be to make this patch provide a single .tbd implementation as independently as possible, and then in separate patches move in the auxiliary (Architecture, Version, Registry, etc.) components later. That would make it significantly easier to provide specific feedback and discussion on whether code is MachO-specific or if some of it could be shared with ELF to reduce code duplication.

The original TAPI is rather large when compared against other llvm libraries, so I'd like to be careful when adding the ELF backend such that it doesn't inflate needlessly. I could adapt the ELF portions to take advantage of existing interfaces after they land, but without more granular review there might be some parts that are specialized and integrated so much that it wouldn't be realistic to do so without completely refactoring existing code.

Nov 6 2018, 1:00 PM

Nov 5 2018

jakehehrlich added a comment to D53945: [TextAPI] TBD Reader/Writer.

General requests

Nov 5 2018, 9:40 AM

Oct 31 2018

jakehehrlich added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

Let's not break compaitableity then and just check the index. The fewer
cases this happens in the better IMO. I guess basically what you had at the
start was actually the best.

Oct 31 2018, 10:50 AM

Oct 30 2018

jakehehrlich accepted D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

I spoke with Roland. Roland basically agrees that there's no good reason for this to function this way (also pointed out that SHN_COMMON is a legacy thing)...but he also agrees that none of that matters because we need to support this case. So LGTM pending anyone else's code/test specific issues.

Oct 30 2018, 3:18 PM
jakehehrlich updated subscribers of D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

Ah fair. That looks like many cases we've unfortunately seen before (see
SHF_LINK and SHF_COMPRESSED) that are not always respected (and thus we
must support the old case as well). So disregard my comment about using the
type. That's what I get for blindly reading the spec.

Oct 30 2018, 12:48 AM

Oct 29 2018

jakehehrlich accepted D53839: [CMake][Fuchsia] Drop the LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT.
Oct 29 2018, 3:50 PM
jakehehrlich added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Looks pretty close. My major concerns are with the currently chosen interface but I think we can shake that out later.

Oct 29 2018, 2:54 PM
jakehehrlich added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

Another thought I had was that we could add an escape hatch for specific symbols the way "--keep" adds an escape hatch for specific sections. It's as if we have a few operations now

Oct 29 2018, 12:19 PM
jakehehrlich added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

I currently view this change as padding the walls for a somewhat improper use of llvm-objcopy. Sort of like how javascript, PHP, or other such languages see a funny operation at runtime and try and pick the most sensible thing to keep your program running when there should probably be an error. In this case I actually view it as worse because this is an operation you might actually want to perform unlike adding 5 and "10 bananas" to get 15 or whatever crazy thing PHP does.

Oct 29 2018, 12:16 PM
jakehehrlich accepted D53733: [llvm-objcopy] Fix --keep-global-symbol/--globalize-symbol for undefined symbols..

This seems correct to me. LGTM.

Oct 29 2018, 11:14 AM

Oct 25 2018

jakehehrlich added a comment to D53667: [llvm-objcopy] Handle -O <format> flag..

I spoke a bit offline with Jordan. I'm going to put a patch up by end of day that refactors things so that this can be done in the Writer. It's a bit hariy but I really don't want setArch or anything like it. The issue we were aware of before was that size was format dependent and we (me really) kluded around it. This actully points out that many other things are as well. The general shift that I've liked here is to slowy remove state from the internal representation and push those things toward constants in the writers. This change I have in mind will remove a lot of state from sections and additionally add that section headers be written on a section by section basis.

Oct 25 2018, 11:57 AM

Oct 24 2018

jakehehrlich closed D52182: [llvm-objcopy] Make -S an alias for --strip-all.
Oct 24 2018, 5:05 PM

Oct 23 2018

jakehehrlich accepted D53163: [llvm-strip] Support -s alias for --strip-all. Make both strip and objcopy case sensitive to support both -s (--strip-all) and -S (--strip-debug)..

Looks like a strict improvement to me and a nice test case fix. Thanks!

Oct 23 2018, 11:38 AM

Oct 15 2018

jakehehrlich added a comment to D53084: [clang-doc] Add unit tests for YAML.

This just seems like a lit test in unit test form, why does this need to use unit tests and not lit tests?

Oct 15 2018, 2:26 PM · Restricted Project
jakehehrlich accepted D53083: [clang-doc] Add unit tests for merging.
Oct 15 2018, 2:22 PM · Restricted Project
jakehehrlich accepted D53081: [clang-doc] Add unit tests for serialization.

LGTM, I'd file a bug about SmallString not taking const char* in its constructor and I'd also put a TODO somewhere to fix that once that's resolved.

Oct 15 2018, 2:13 PM · Restricted Project

Oct 9 2018

jakehehrlich added a comment to D51009: [opt] Change the parameter of OptTable::PrintHelp from Name to Usage and don't append "[options] <inputs>".

.

Oct 9 2018, 5:28 PM
jakehehrlich committed rL344080: [llvm-objcopy] Make -S an alias for --strip-all.
[llvm-objcopy] Make -S an alias for --strip-all
Oct 9 2018, 2:16 PM
jakehehrlich added a comment to D52182: [llvm-objcopy] Make -S an alias for --strip-all.

Hey, sorry. I've been gone for 2 weeks between ICFP/Strange Loop and being in Seattle. Commit this now.

Oct 9 2018, 12:19 PM
jakehehrlich accepted D53029: [llvm-objcopy] Add -F|--target compatibility.

I think this is probably fine. I think having the error is better than not having it. I'm just concerned someone somewhere is using GNU objcopy in the way mentioned in the error. I'm fine treating this as an error and only switching behavior if and when someone hits an issue.

Oct 9 2018, 12:18 PM

Sep 28 2018

jakehehrlich added a comment to D52660: [CMake][Fuchsia] Use unstable libc++ ABI for Fuchsia toolchain.

We have no such desire. I think we're fine being the test subjects for
ABIv2 however.

Sep 28 2018, 5:09 PM

Sep 20 2018

jakehehrlich accepted D52328: [llvm-objcopy/llvm-strip]: handle --version.

So I'm good with this BTW.

Sep 20 2018, 4:59 PM

Sep 17 2018

jakehehrlich added a comment to D52180: [llvm-objcopy] Change --only-keep to --only-sections.

Who put me in charge of this? I'm clearly unqualified.

Sep 17 2018, 12:54 PM
jakehehrlich created D52182: [llvm-objcopy] Make -S an alias for --strip-all.
Sep 17 2018, 12:05 PM
jakehehrlich created D52180: [llvm-objcopy] Change --only-keep to --only-sections.
Sep 17 2018, 11:24 AM
jakehehrlich added a comment to D52163: -S as an alias for --strip-all-gnu.

-S should be an alias for --strip-all not --strip-all-gnu. There's seldom if ever going to be someone who rely's on the difference between the two. --strip-all-gnu exists as a way to allow people to hack llvm-objcopy into their system for entirely imagined use cases that we have not thus far actually seen. I'll be updating this patch.

Sep 17 2018, 11:07 AM

Sep 12 2018

jakehehrlich accepted D51961: [objcopy] make objcopy follow program header standards.

LGTM.

Sep 12 2018, 10:25 AM

Sep 11 2018

jakehehrlich requested changes to D51961: [objcopy] make objcopy follow program header standards.

I'll LGTM after Roland's fixes are done since I agree, that would be nice to also fix.

Sep 11 2018, 7:00 PM
jakehehrlich added inline comments to D51961: [objcopy] make objcopy follow program header standards.
Sep 11 2018, 6:53 PM
jakehehrlich accepted D51961: [objcopy] make objcopy follow program header standards.

Oh also LGTM

Sep 11 2018, 6:10 PM
jakehehrlich added a comment to D51961: [objcopy] make objcopy follow program header standards.

When you write your diff use git diff -U<some big number> origin/master so that the diff is relative to what's upstream not your last change locally. I think git show might be understood by fabricator as well but I'm not 100% sure.

Sep 11 2018, 6:06 PM
jakehehrlich added a comment to D51961: [objcopy] make objcopy follow program header standards.

A couple nits but this LGTM.

Sep 11 2018, 4:56 PM

Sep 10 2018

jakehehrlich added inline comments to D51841: [llvm-objcopy] Dwarf decompression support. .
Sep 10 2018, 2:26 PM
jakehehrlich added a comment to D51841: [llvm-objcopy] Dwarf decompression support. .

So I was thinking that we might try and detect compressed section at section construction time instead. The we can implement decompression in terms of CompressedSection...the only blemish seems to be that CompressedSection always allocates. I'm not super sure what the right solution to that is.

Sep 10 2018, 12:21 PM

Sep 7 2018

jakehehrlich updated subscribers of D51792: [AArch64] Attempt to parse expressions as adr/adrp operands.

Awesome! I'm looking at this now.

Sep 7 2018, 3:30 PM

Sep 4 2018

jakehehrlich accepted D51660: [llvm-strip] Support stripping multiple input files.

Whoops...I meant to say the following already. I was confused why it was missing my LGTM.

Sep 4 2018, 8:11 PM
jakehehrlich accepted D51493: [llvm-strip] Allow copying relocation sections without symbol tables..

Confirmed, using head solves this issue; I was just using a very stale checkout.

Sep 4 2018, 3:19 PM
jakehehrlich added a comment to D51493: [llvm-strip] Allow copying relocation sections without symbol tables..

Hmm...this is odd...maybe I missed a change to yaml2obj. I'm rebuilding now.

Sep 4 2018, 2:12 PM
jakehehrlich added a comment to D51493: [llvm-strip] Allow copying relocation sections without symbol tables..

When I run that yaml2obj test I actually get the following error rather than a silent failure.

Sep 4 2018, 2:09 PM
jakehehrlich added a comment to D51493: [llvm-strip] Allow copying relocation sections without symbol tables..

Did you confirm that yaml2obj respects the explicit Link field?

Sep 4 2018, 1:56 PM
jakehehrlich added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

Ah so I think this might be an issue with builders that don't build with zlib or that link zlib in in strange ways. I remember I mentioned (many weeks ago) that we'd need to #ifdef some of this stuff into compartments so that it didn't affect non zlib builds. I didn't remember to check for that before giving the LGTM. That is certainly killing at least some bots. I think the downstream bots that Petr mentioned are however actually linking in zlib. It isn't clear why this is breaking those but I'll try and look into that today.

Sep 4 2018, 10:45 AM
jakehehrlich requested changes to D51493: [llvm-strip] Allow copying relocation sections without symbol tables..

Sorry, I didn't mean to accept this change. I was about to and then thought about the Link issue I mentioned in the test. I don't know how to cancel an approval in phabricator so I'm marking it as "request changes". Nothing is critically wrong here, I just want to make sure the test works as intended.

Sep 4 2018, 10:36 AM
jakehehrlich accepted D51493: [llvm-strip] Allow copying relocation sections without symbol tables..
Sep 4 2018, 10:36 AM
jakehehrlich added inline comments to D51493: [llvm-strip] Allow copying relocation sections without symbol tables..
Sep 4 2018, 10:31 AM

Aug 31 2018

jakehehrlich accepted D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

I am satisfied that James' two remaining issues (as I understand them, more efficient looping in compressSections and minimal use of MCTargetOptions) have been solved and that mine have been solved.

Aug 31 2018, 1:19 PM

Aug 29 2018

jakehehrlich accepted D51468: [sanitizer] Transition to new _zx_vmar_... calls.
Aug 29 2018, 6:24 PM
jakehehrlich added a comment to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..

This is looking pretty close to good to me. I have a couple more nits about how alignment and RemovePred should work but other than that I think I'm good. If those are fixed and someone else accepts this revision before I get back to this feel free to land it without further input from me.

Aug 29 2018, 3:11 PM

Aug 28 2018

jakehehrlich added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Aug 28 2018, 2:18 PM
jakehehrlich added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Aug 28 2018, 1:25 PM

Aug 27 2018

jakehehrlich added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Aug 27 2018, 3:13 PM
jakehehrlich added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Aug 27 2018, 1:41 PM

Aug 24 2018

jakehehrlich added inline comments to D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections..
Aug 24 2018, 5:12 PM