Page MenuHomePhabricator

jakehehrlich (Jake Ehrlich)
User

Projects

User does not belong to any projects.

User Details

User Since
May 16 2017, 11:34 AM (101 w, 1 d)

Recent Activity

Today

jakehehrlich added a comment to D60974: Clang IFSO driver action..

@jakehehrlich - when do you expect to have your idea put up? I don't think that it is fair to have this wait until you have time to put something up that can be discussed. I think that getting this working and then iterating on it and migrating it over to some shared representation is something which we could do - that tends to be a common thing that I have seen happen multiple times with the necessary work never materialising. Re-use of the YAML structure means that we can iterate and identify the pieces that are necessary, though, I expect that largely, what will be needed is the name, the binding, the visibility, possibly the size (for TBEs), the section, and the type, at least for anything which adheres to the GABI. If you have extensions outside of GABI, this will need to be adjusted.

Wed, Apr 24, 3:05 PM · Restricted Project
jakehehrlich added a comment to D60974: Clang IFSO driver action..

To be clear I'm only really giving my thumbs up to a specific high level plan that I believe handles a case that llvm-elfabi wouldn't otherwise handle on its own. I consider the specifics like the output format still in the air right now. I strongly believe that a unique yaml or json format should be used for this. We should discuss the specifics of what should go in that format and have a discussion on that format. The trick is to capture everything in .o files that could wind up mattering to a .tbe file and nothing more. I haven't thought though what that entails completely yet but I'll think about it and throw up my proposal. Once we have at least one proposal we can start iterating on the specifics of it.

Wed, Apr 24, 2:16 PM · Restricted Project

Yesterday

jakehehrlich added a comment to D60974: Clang IFSO driver action..

The strong motivation here is to avoid having the list be maintained outside of the source code. I am not too strongly tied to the YAML approach as long as the binaries that we get at the end are equivalent to the pruned ELF binaries from the YAML2ELF approach. However, the trade off here is valuable - it allows for the source code to be the source of truth for the interface set. The intermediate emission is due to the fact that this requires program level visibility and having the intermediates an extra step to merge is relatively simpler. In fact, you could emit the TBE files from this and check that into the tree if you really wanted (but allowing the source to remain the source of truth I think is a valid and valuable trade off). The reason for the selection of YAML is simply pragmatism - that is already available in the tree. If the TBE linker is available now, I have no issue with that being the intermediate representation (assuming that the representation is easy to parse programatically).

Tue, Apr 23, 3:00 PM · Restricted Project
jakehehrlich added a comment to D60974: Clang IFSO driver action..

I actually don't see why something like this couldn't be used in both scenarios (ie some users generating the files at build time, and others using -emit-ifso to periodically generate and land changes to checked in files).

Tue, Apr 23, 1:55 PM · Restricted Project
jakehehrlich added a comment to D60974: Clang IFSO driver action..

Why is this better than producing output from the output of the static linker at the end? Also how do we plan on integrating this with llvm-elfabi so that we don't have unnecessarily duplicated efforts? Why does that tool not suffice?

Tue, Apr 23, 1:12 PM · Restricted Project

Mon, Apr 22

jakehehrlich added a comment to D60974: Clang IFSO driver action..

I'm not misunderstanding, please seem my post on llvm-dev where I layout the different issues involved. You're underestimating the complexities involved here I think. Trivializing what the linker does to "the linker just merges these" is a gross oversimplification that doesn't account for symbol consuming vs exporting, size, alignment, visibility, linkage, symbol type, symbol versioning (which is impossible to handle here without consuming version scripts as was pointed out by someone else), which symbols are untimely exported/consumed or even intermediate operations on .o files via tools like objcopy (which is uncommon and shouldn't be a hard blocker here but something to think about). Also all linkers behave just a bit differently with respect to these issues so 1) the reality is that the thing that merges these will behave just a bit differently than any existing linker but 2) even if it trys to match one linker it will fail to match the other so it will only be compaitable in all cases with atmost one linker (but probably none when edge cases are considered. I also already pointed out the issue with having to search all source files on the tree. I agree that if you were to use the linker to accomplish this that would would have to look at all source files. Further more many flags passed to the linker untimely affect these things making matters more complicated. The "linker" you describe for these cases is far from "cat".

Mon, Apr 22, 7:11 PM · Restricted Project
jakehehrlich added a comment to D60974: Clang IFSO driver action..

There are a few that I have in mind.

  1. -emit-ifso could be added to a build to produce .so files as part of a device SDK (where we don't want to ship the runnable bits in the SDK, we ship those on the device updates).
Mon, Apr 22, 3:40 PM · Restricted Project
jakehehrlich added a comment to D60270: [llvm-objcopy] Add support for Intel HEX input/output format.

There's a lot of code to review here. I'll keep reviewing it everyday but this is going to take a while to review. Any help on splitting this up and making into smaller chunks would be helpful. Splitting reading and writing up into two separate patches would be helpful and removing features that we can add later would be helpful.

Mon, Apr 22, 1:16 PM
jakehehrlich added a comment to D60974: Clang IFSO driver action..

Can you elaborate on the use case for this? Like can you explain end to end how this would be used?

Mon, Apr 22, 12:42 PM · Restricted Project
jakehehrlich added inline comments to D60042: [llvm-objcopy] Add --prefix-alloc-sections.
Mon, Apr 22, 12:21 PM · Restricted Project

Wed, Apr 17

jakehehrlich added a comment to D60439: [llvm-objcopy] Accept --long-option but not -long-option.

I feel like we didn't get a solid conclusion here but that the general consensus is that this change is more ideal. I think the argument that this is incompatible with merged short options and that that might have compatibility issues is a strong argument personally. The other not-quite-conclusion-but-leaning-that-way seems to be that we can't really make this same choice for all binaries. Do we feel like the conclusion of the llvm-dev post supports yet further fragmenting the tools? I don't know and I'm concerned about making that choice. We have to way tool consistency with GNU compatibility here and that's a hard choice. I'll add an email later to see if we can flesh this out more.

Wed, Apr 17, 11:00 AM · Restricted Project
jakehehrlich added a comment to D60324: [llvm-objcopy] Add switch to allow removing referenced sections.

This seems like a good idea and looks good from an architectural standpoint. Don't wait on me to land it (e.g. I'm not giving an LGTM because I haven't reviewed it properly but I've reviewed it enough to know that this is both a) a good idea and b) nothing bad is being introduced). I'm positive Jordan and George's review have already filled what I haven't done.

Wed, Apr 17, 10:54 AM · Restricted Project
jakehehrlich added a comment to D60042: [llvm-objcopy] Add --prefix-alloc-sections.

A few comments.

Wed, Apr 17, 10:47 AM · Restricted Project
jakehehrlich added a comment to D60825: [llvm-objcopy] - Check dynamic relocation sections for broken references..

So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.

Wed, Apr 17, 8:18 AM

Tue, Apr 9

jakehehrlich added a comment to D60189: [llvm-objcopy] Simplify SHT_NOBITS -> SHT_PROGBITS promotion.

I haven't bread or considered this patch. I just want to clarify when I think we should make changes of this type.

Tue, Apr 9, 4:08 AM · Restricted Project

Wed, Apr 3

jakehehrlich added a comment to D60131: [ELF] Change default output section type to SHT_PROGBITS.

This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?

Wed, Apr 3, 6:31 AM · Restricted Project
jakehehrlich added a comment to D60042: [llvm-objcopy] Add --prefix-alloc-sections.

Ah well then we should implement that behavior. We should get rid of the n^2 loop however.

Wed, Apr 3, 12:00 AM · Restricted Project

Tue, Apr 2

jakehehrlich requested changes to D60042: [llvm-objcopy] Add --prefix-alloc-sections.

I'm not sure I like renaming and I'm not sure anyone requires that behavior. It also introduces n^2 behavior which I don't like. That should be removed at the very least.

Tue, Apr 2, 11:02 PM · Restricted Project

Wed, Mar 27

jakehehrlich added a comment to D59531: [ELF] Produce multiple PT_NOTE segments as needed.

Can you ask Roland how valid it is for a SHT_NOTE section to have sh_size%sh_align != 0? If the scenario isn't realistic, can we just merge adjacent SHF_ALLOC SHT_NOTE with the same sh_align? If there is one, I believe ld.bfd and gold (binutils-gdb trunk) can create PT_NOTE segments with unparsable padding.

Wed, Mar 27, 4:16 PM · Restricted Project
jakehehrlich updated the diff for D59531: [ELF] Produce multiple PT_NOTE segments as needed.

Added Roland's proposed fix

Wed, Mar 27, 4:14 PM · Restricted Project

Mar 22 2019

jakehehrlich closed D59033: [llvm-objcopy] Make .build-id linking atomic.

Right sorry about that.

Mar 22 2019, 12:49 PM · Restricted Project
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

I can think of two ways

Mar 22 2019, 12:47 PM · Restricted Project
jakehehrlich added a comment to D59531: [ELF] Produce multiple PT_NOTE segments as needed.

Roland pointed out that this will be a regression after those special high alignment notes are flushed from the ecosystem (this is apparently a recent addition and the issue will be resolved in the months to come). A possibility is that if we see a note section with greater than 4-byte alignment then we use this method but otherwise fallback to the old method. This means that on Linux binaries for the time being (while this new ABI thing is being flushed out of the ecosystem) will be slightly larger but as soon as its fixed ld.lld will be back to fully functioning. This is a general strategy too because no matter size the notes are, the reader will bump by 4-byte alignment. That seems fine to me and I'll upload that change today hopefully.

Mar 22 2019, 12:25 PM · Restricted Project
jakehehrlich added a comment to D59483: [llvm-objcopy]Preserve data in segments not covered by sections.

Ok, I think how the test functions is fine and it's probably a good test. It certainly tests every case I'd consider. I haven't checked that all the offsets are correct but they don't look wrong at a glance.

Mar 22 2019, 12:16 PM · Restricted Project

Mar 21 2019

jakehehrlich accepted D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry..

LGTM as well. Thanks for checking the spec!

Mar 21 2019, 2:43 PM · Restricted Project

Mar 19 2019

jakehehrlich added a comment to D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry..

The current implementation in this change was what I had in mind but I have questions now. I think this implementation is wrong

Mar 19 2019, 5:38 PM · Restricted Project
jakehehrlich updated the diff for D59531: [ELF] Produce multiple PT_NOTE segments as needed.

Use one PT_NOTE per SHT_NOTE now instead of trying to minimize the number by assuming the layout algorithm. Changed tests to match this reality.

Mar 19 2019, 5:25 PM · Restricted Project
jakehehrlich added a comment to D59531: [ELF] Produce multiple PT_NOTE segments as needed.

We don't know addresses at this point, and indeed we can't because the layout of the rest of the output file depends on how many program headers there are, which is decided by this function.

Mar 19 2019, 2:54 PM · Restricted Project
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

If you think about TLS as being in a sort of different address space (which was not a way I had thought about this before) then this is the function that determines if a section is in the same address area as a segment so its a semantically consistent idea I think. Layout should also always succeed because the PT_TLS will be given the respective PT_LOAD as a parent which will fix its offset correctly and then any sections assigned only to PT_TLS and not a PT_LOAD will still be laid out correctly (which should always be the case for any overlapping segment, we should have tests for this already). We are however changing what this function means even if in practice it's true/false in almost all of the same cases. For non-NOBITS Segment.VAddr - Section.Addr == Segment.Offset - Section.OriginalOffset and those sections will never fall in the range past Segment.FileSize so the predicate should return true in all the same cases for non-NOBITS. It would also now assign sections to the "expected" segment even if today that has no affect one way or the other.

Mar 19 2019, 2:45 PM · Restricted Project
jakehehrlich added a comment to D59531: [ELF] Produce multiple PT_NOTE segments as needed.

I think pcc's case is valid and we should be able to update the script to account for it (I think anyway). This issue arises when the offset/vaddr of the output section is already known to be larger than gap that note traversal would assume. My current algorithm makes assumptions about how section layout occurs.

Mar 19 2019, 1:04 PM · Restricted Project
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

I contest that this change is correct. This predicate is specifically designed to capture which bytes overlap in the file, not the run time semantic goal you're looking for. I'm also concerned that it will interact badly with PT_TLS, . It was for this exact reason (handling of PT_TLS) that we chose to use Offset and Filesize *instead* of VAddr and MemSize as was my first instinct.

Mar 19 2019, 12:51 PM · Restricted Project
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

I mean in the handler for the option you can re-traverse the program headers for each section and handle your check.

Mar 19 2019, 11:33 AM · Restricted Project

Mar 18 2019

jakehehrlich created D59531: [ELF] Produce multiple PT_NOTE segments as needed.
Mar 18 2019, 7:20 PM · Restricted Project
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

For your use case you can simply traverse the program headers for each to see if a section is indeed loadable. e.g. you want to strip all non-loadable allocated sections. That seems like a good thing but this probably isn't the right way to handle it IMO.

Mar 18 2019, 1:40 PM · Restricted Project
jakehehrlich committed rG5049c3422d26: [llvm-objcopy] Make .build-id linking atomic (authored by jakehehrlich).
[llvm-objcopy] Make .build-id linking atomic
Mar 18 2019, 1:36 PM
jakehehrlich committed rL356404: [llvm-objcopy] Make .build-id linking atomic.
[llvm-objcopy] Make .build-id linking atomic
Mar 18 2019, 1:36 PM
jakehehrlich added inline comments to D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry..
Mar 18 2019, 12:22 PM · Restricted Project
jakehehrlich added a comment to D59483: [llvm-objcopy]Preserve data in segments not covered by sections.

The code looks good to me but the test is going to take me a while to review. I don't think I'll get to this today.

Mar 18 2019, 12:17 PM · Restricted Project
jakehehrlich added a reviewer for D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets.: jakehehrlich.
Mar 18 2019, 12:03 PM · Restricted Project
jakehehrlich added inline comments to D59351: [llvm-objcopy] Add --update-section.
Mar 18 2019, 12:00 PM
jakehehrlich added a comment to D58426: llvm-objcopy: Change sectionWithinSegment() to use virtual addresses instead of file offsets..

If .bss has the desired offset this will work. Also .tbss, .tdata, PT_TLS, and PT_LOAD interact in very strange ways that we need to be careful about. In particular the TLS image will exist in a PT_LOAD read-only segment but the PT_TLS segment will have a bogus vaddr and the .tbss will extend into the part of the PT_LOAD that covers .rodata, .dynstr, etc...

Mar 18 2019, 11:52 AM · Restricted Project
jakehehrlich added a comment to D59351: [llvm-objcopy] Add --update-section.

Rather than updating a section in place we should extend the "replaceSectionReferences" method to work in every section type, and then swap the object out for a new object rather than updating in place.

Mar 18 2019, 11:43 AM
jakehehrlich added a comment to D59488: [llvm-objcopy] - Calculate the string table section sizes correctly..

Maybe we should make "prepareForLayout" a top level operation that we can just run on all sections?

Mar 18 2019, 11:24 AM · Restricted Project

Mar 14 2019

jakehehrlich added inline comments to D59033: [llvm-objcopy] Make .build-id linking atomic.
Mar 14 2019, 3:12 PM · Restricted Project
jakehehrlich updated the diff for D59033: [llvm-objcopy] Make .build-id linking atomic.
Mar 14 2019, 3:03 PM · Restricted Project
jakehehrlich added a comment to D59372: [yaml2obj]Allow explicit setting of p_filesz, p_memsz, and p_offset.

Oh also we can add PHDR tests to llvm-objcopy with this!

Mar 14 2019, 12:28 PM · Restricted Project
jakehehrlich added a comment to D59372: [yaml2obj]Allow explicit setting of p_filesz, p_memsz, and p_offset.

I think there are some really funky tests in llvm-objcopy that we can fix now but it's going to take a careful eye to see which ones.

Mar 14 2019, 12:28 PM · Restricted Project
jakehehrlich accepted D59372: [yaml2obj]Allow explicit setting of p_filesz, p_memsz, and p_offset.

This is much needed. Thanks!

Mar 14 2019, 12:26 PM · Restricted Project

Mar 13 2019

jakehehrlich added inline comments to D59293: [llvm-objcopy]Don't implicitly strip sections in segments.
Mar 13 2019, 2:47 PM · Restricted Project
jakehehrlich added inline comments to D59033: [llvm-objcopy] Make .build-id linking atomic.
Mar 13 2019, 1:43 PM · Restricted Project
jakehehrlich accepted D59293: [llvm-objcopy]Don't implicitly strip sections in segments.

This isn't precisely what I had in mind but I think its a strict improvement. Tests look good and document the specific behavior well. I think the behavior with zeroing out sections in explicit cases or where we're looking to match GNU behavior precisely is a good idea (and would have been harder to impossible with what I had in mind). The only one I question is --strip-non-alloc but that was something I invented that turned out not to be useful. We could probably remove that and no one would care. There's not standard or use case to guide what should happen there. The guiding principal should probably be "when it doesn't make people scratch their heads, don't remove stuff in segments" which this change adhears to.

Mar 13 2019, 1:43 PM · Restricted Project

Mar 11 2019

jakehehrlich added a comment to D59126: llvm-objcopy: Remove unused field. NFCI..

So this change and the issue that James hit made me think about a few short comings in llvm-objcopy that I think should be addressed.

Mar 11 2019, 12:02 PM · Restricted Project

Mar 8 2019

jakehehrlich accepted D59127: [CMake] Support stripping and linking output to .build-id directory.

LGTM

Mar 8 2019, 12:59 PM · Restricted Project, Restricted Project
jakehehrlich added a comment to D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..

This looks good to me but it is perhaps confusing that the replaceSectionReferences method is unimplemented for many types where it makes sense. Implementing it for all types makes --only-keep-debug possible for instance.

Mar 8 2019, 12:00 PM · Restricted Project
jakehehrlich accepted D58960: [llvm-objcopy] - Fix --compress-debug-sections when there are relocations..
Mar 8 2019, 12:00 PM · Restricted Project
jakehehrlich updated the diff for D59033: [llvm-objcopy] Make .build-id linking atomic.
  1. Fixed typos
  2. Added context (sorry about that, rookie mistake)
Mar 8 2019, 11:54 AM · Restricted Project

Mar 7 2019

jakehehrlich added a comment to D59120: [ELF] Sort notes by alignment in decreasing order.

Would be nice to have a test where 2 sections are out of order, and this change puts them into the correct order. It's not clear from this if the build ID section is naturally placed at the end or not.

Mar 7 2019, 5:18 PM · Restricted Project

Mar 6 2019

jakehehrlich created D59033: [llvm-objcopy] Make .build-id linking atomic.
Mar 6 2019, 11:12 AM · Restricted Project

Feb 20 2019

jakehehrlich accepted D58445: [yaml2obj]Allow symbol Index field to take values lower than SHN_LORESERVE.

This emphatically looks good to me. Thanks for doing this.

Feb 20 2019, 1:51 PM · Restricted Project
jakehehrlich accepted D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.

This change looks good to me and the semantics seem sound. I'd still appreciate a use case however. This is small and compact enough and I can imagine you might want to do this in cases where a process doesn't fix itself up at all and consequently there are multiple valid entry points but by and large this seems quite odd to me.

Feb 20 2019, 1:49 PM · Restricted Project
jakehehrlich added a comment to D58234: [llvm-objcopy] Add --add-symbol.

Sorry to be the Grinch again but what is the use case for this? This isn't a feature in GNU objcopy as far as I can tell and the semantics of such a change are quite involved. I think I'd prefer to discuss this in llvm-dev first. I belive this can be given meaningful and consistent semantics and I think it could have a use case but right now I don't see one. This seems like a complicated feature to add without a use case in mind.

Feb 20 2019, 1:40 PM · Restricted Project
jakehehrlich updated subscribers of D58296: [llvm-objcopy] Make removeSectionReferences batched.

If we're just giving a set we might as well give the user a function instead right? That's a bit more general of an interface and easier to use. many-sections.o.gz is smaller than 3.3 MB right? I went to a lot of pains to make sure that I was uploading as small a file as was possible. Do we have large file support

Feb 20 2019, 1:29 PM · Restricted Project

Feb 18 2019

jakehehrlich added a comment to D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start.
  1. What's the use case for this?
  2. What's the interaction for program headers?
Feb 18 2019, 9:43 AM · Restricted Project

Feb 14 2019

jakehehrlich edited reviewers for D58116: [llvm-objcopy] Improve section removal, added: jakehehrlich, mcgrathr; removed: espindola.
Feb 14 2019, 3:12 PM
jakehehrlich updated subscribers of D58116: [llvm-objcopy] Improve section removal.

cc @mcgrathr who can comment more in depth on how this works in GNU objcopy and how useful it is. He already noted that anything relaying on segment resizing behavior from GNU objcopy is already fragile.

Feb 14 2019, 3:10 PM
jakehehrlich added a comment to D58116: [llvm-objcopy] Improve section removal.

Can we get an explicit real use case for this or a case where not removing these sections is detrimental? I believe this to be fundamentally wrong otherwise. Imagine what would happen if you first ran --strip-sections and then ran literally anything again. It's also a primary tenant so far that we not modify program headers. That's how we maintain that stripping not affect run time behavior so trivially. This diff changes that.

Feb 14 2019, 3:05 PM

Jan 29 2019

jakehehrlich accepted D57432: [CMake][Fuchsia] Re-enable iOS runtimes for Fuchsia standard.
Jan 29 2019, 6:03 PM
jakehehrlich accepted D57431: [CMake][Fuchsia] Enable hermetic static libunwind for Fuchsia.
Jan 29 2019, 5:59 PM
jakehehrlich accepted D57423: [llvm-objcopy][NFC] More error propagation.

OMG thank you. I've been wanting this to happen so badly. LGTM.

Jan 29 2019, 5:03 PM

Jan 28 2019

jakehehrlich added a comment to D57198: [llvm-objcopy] Implement --set-section-flags..
  • Add --set-section-flags to the list of unimplemented COFF methods
Jan 28 2019, 3:29 PM
jakehehrlich accepted D57198: [llvm-objcopy] Implement --set-section-flags..

Even though this is a large change I think I'm happy with it. Everything was either factoring out common code or handling error messages. I didn't see anything about how error messages were handled that was objectionable.

Jan 28 2019, 3:27 PM

Jan 25 2019

jakehehrlich added a comment to D56806: [llvm-objcopy] Fix crash when writing empty binary output.

LGTM.

Jan 25 2019, 1:47 PM
jakehehrlich accepted D56806: [llvm-objcopy] Fix crash when writing empty binary output.
Jan 25 2019, 1:47 PM

Jan 24 2019

jakehehrlich added inline comments to D56806: [llvm-objcopy] Fix crash when writing empty binary output.
Jan 24 2019, 6:22 PM
jakehehrlich accepted D56806: [llvm-objcopy] Fix crash when writing empty binary output.

Does overwriting a file create a new file? --build-id-link-dir and friends relay on that behavior. In general tools should *never* modify a file in place. Consider any build that uses GN (like Fuchsia, or Chromeium, and now optionally llvm/clang). Say they have a rule (like how we strip in Fuchsia) that writes a file. Now say a copy rule is used to put that somewhere else or you use --build-id-link-dir to copy into a .build-id directory (both happen in Fuchsia). The correct behavior should be for a new file to be created on disk, not modification of the existing one. This for instance keeps the file in the .build-id directory correct. I think this truncates the same existing inode.

Jan 24 2019, 10:56 AM
jakehehrlich requested changes to D56806: [llvm-objcopy] Fix crash when writing empty binary output.

Sorry, meant to request changes.

Jan 24 2019, 10:56 AM
jakehehrlich added a comment to D57146: [llvm-objdump] - Print LMAs when dumping section headers..

The code change looks basically fine, but I'm wondering if we really want this? For many (most?) use cases, the LMA is the same as the VMA, so the extra column is just useless noise. I know we want to broadly be consistent with GNU tools, but I wonder if that's the case when it doesn't make that much sense?

What do you think about the following approach:

  1. If there are any segments with different p_vaddr and p_paddr, go with your suggestion.
  2. If a user specifies a switch, e.g. --show-lma, also go with your suggestion.
  3. Otherwise, only print the VMA.

    The issue with this is that it might break parsers which are written against GNU objdump's behaviour. I'm not sure if this is an issue or not? The switch is designed to mitigate that problem.

    It's probably worth getting some others to give their opinion on this.
Jan 24 2019, 10:42 AM

Jan 23 2019

jakehehrlich added inline comments to D55839: [elfabi] Add support for writing ELF header for binary stubs.
Jan 23 2019, 12:46 PM
jakehehrlich accepted D56031: [elfabi] Add support for reading dynamic symbols from binaries.

Pending 1 nit, LGTM.

Jan 23 2019, 12:20 PM

Jan 18 2019

jakehehrlich added inline comments to D56031: [elfabi] Add support for reading dynamic symbols from binaries.
Jan 18 2019, 1:20 PM
jakehehrlich accepted D55852: [elfabi] Add support for reading DT_NEEDED from binaries.

LGTM

Jan 18 2019, 11:45 AM

Jan 4 2019

jakehehrlich accepted D55629: [elfabi] Add support for reading DT_SONAME from binaries.

Super informative error messages!

Jan 4 2019, 2:28 PM
jakehehrlich accepted D56319: [llvm-readobj] Don't print '@' at end of dynamic symbol names for symbols with no version.

LGTM as well.

Jan 4 2019, 1:24 PM

Jan 3 2019

jakehehrlich requested changes to D55629: [elfabi] Add support for reading DT_SONAME from binaries.

As discussed offline I realized I missed something.

Jan 3 2019, 5:05 PM
jakehehrlich accepted D55629: [elfabi] Add support for reading DT_SONAME from binaries.

LGTM. However make sure to at least CC some people outside, if not add them as reviewers on everything.

Jan 3 2019, 4:29 PM

Jan 2 2019

jakehehrlich added a comment to D56211: [llvm-objcopy][ELF] Implement a mutable section visitor that updates size-related fields (Size, EntrySize, Align) before layout..

This is a preety nice solution. I'd been working on removing a lot of the fields from SectionBase, and making an object that had all the sizes of various things listed in it, and constructing that from the ELFT and moving all the calculation of such things into the Writer. That changes kept getting larger and more hairy. This is still quite clean and far less invasive. Also I think a mutable section visitor is a generically useful thing to have around. Nice work. I'd wait for one more LGTM. Sorry I wasn't able to get the change I promised out in time.

Jan 2 2019, 12:04 PM

Dec 20 2018

jakehehrlich added a comment to D55881: [llvm-objcopy] [COFF] Add support for removing symbols.

Head's up I'm about to go home for the evening and I'm pretty well off for until the new year after that. I think others are in a similar boat. I'm not ready to LGTM this yet and I don't have time to give more review so this might have to wait until January 2nd.

Dec 20 2018, 4:05 PM
jakehehrlich added inline comments to D55629: [elfabi] Add support for reading DT_SONAME from binaries.
Dec 20 2018, 12:20 PM

Dec 19 2018

jakehehrlich added inline comments to D55839: [elfabi] Add support for writing ELF header for binary stubs.
Dec 19 2018, 2:57 PM
jakehehrlich added inline comments to D55881: [llvm-objcopy] [COFF] Add support for removing symbols.
Dec 19 2018, 2:48 PM
jakehehrlich added a comment to D55881: [llvm-objcopy] [COFF] Add support for removing symbols.

One broad comment is that we should probably implement these in different patches. Just pick one for now and we can build up to the others.

Dec 19 2018, 2:15 PM
jakehehrlich accepted D55886: [llvm-objcopy] - Do not drop the OS/ABI and ABIVersion fields of ELF header.

LGTM, I'd like James's approval and my only comment is about a feature we can add after this if its needed. James is also out on leave so go ahead and land this.

Dec 19 2018, 2:11 PM
jakehehrlich accepted D54674: [llvm-objcopy] First bits for MachO .

I think this looks pretty good. I'd like an explicit public approval from someone who knows MachO better but pending that and a few relatively minor requests I think I'm happy. I've gone over the high level structure and approve of it. I see major obvious issues in the code, it looks preety good to me. I'll defer to someone else on the MachO details. Testing looks pretty good but I don't know the full space very well so I'd like a MachO person to look at that as well.

Dec 19 2018, 1:19 PM · Restricted Project

Dec 18 2018

jakehehrlich added inline comments to D55629: [elfabi] Add support for reading DT_SONAME from binaries.
Dec 18 2018, 5:14 PM
jakehehrlich added a comment to D55852: [elfabi] Add support for reading DT_NEEDED from binaries.

Other than me realizing I made a mistake in previous reviews and a minor little issue (NeededLibCount), this LGTM.

Dec 18 2018, 5:06 PM
jakehehrlich added a comment to D55839: [elfabi] Add support for writing ELF header for binary stubs.

The structure looks most good but I've got a lot of little requests.

Dec 18 2018, 4:47 PM
jakehehrlich accepted D54939: [llvm-objcopy] Initial COFF support.

High level structure/interface looks good to me. I don't see in obvious code level issues. I'll trust @rnk's assessment of the COFF specific parts of this. I haven't reviewed the testing inputs but it looks like you're able to copy some substantial object files which looks like a solid start. I'll trust that James and Alex have looked at enough of this to fill in the gaps that I might have missed. We can start iterating now if there are any issues. Land it.

Dec 18 2018, 3:54 PM

Dec 12 2018

jakehehrlich accepted D55619: [elfabi] Add option to manually specify file read format.

LGTM

Dec 12 2018, 2:28 PM

Dec 11 2018

jakehehrlich accepted D55352: [elfabi] Introduce tool for ELF TextAPI.
Dec 11 2018, 6:46 PM
jakehehrlich added a comment to D55352: [elfabi] Introduce tool for ELF TextAPI.

I'd like someone outside of our team like @jhenderson to review this. Landing a new tool without some outside eyes looking at it isn't very community friendly IMO.

Dec 11 2018, 6:46 PM