Page MenuHomePhabricator

jakehehrlich (Jake Ehrlich)
User

Projects

User does not belong to any projects.

User Details

User Since
May 16 2017, 11:34 AM (92 w, 3 d)

Recent Activity

Wed, Feb 20

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.

Wed, Feb 20, 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.

Wed, Feb 20, 1:49 PM
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.

Wed, Feb 20, 1:40 PM
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

Wed, Feb 20, 1:29 PM · Restricted Project

Mon, Feb 18

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?
Mon, Feb 18, 9:43 AM

Thu, Feb 14

jakehehrlich edited reviewers for D58116: [llvm-objcopy] Improve section removal, added: jakehehrlich, mcgrathr; removed: espindola.
Thu, Feb 14, 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.

Thu, Feb 14, 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.

Thu, Feb 14, 3:05 PM

Tue, Jan 29

jakehehrlich accepted D57432: [CMake][Fuchsia] Re-enable iOS runtimes for Fuchsia standard.
Tue, Jan 29, 6:03 PM
jakehehrlich accepted D57431: [CMake][Fuchsia] Enable hermetic static libunwind for Fuchsia.
Tue, Jan 29, 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.

Tue, Jan 29, 5:03 PM

Mon, Jan 28

jakehehrlich added a comment to D57198: [llvm-objcopy] Implement --set-section-flags..
  • Add --set-section-flags to the list of unimplemented COFF methods
Mon, Jan 28, 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.

Mon, Jan 28, 3:27 PM

Fri, Jan 25

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

LGTM.

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

Thu, Jan 24

jakehehrlich added inline comments to D56806: [llvm-objcopy] Fix crash when writing empty binary output.
Thu, Jan 24, 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.

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

Sorry, meant to request changes.

Thu, Jan 24, 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.
Thu, Jan 24, 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
jakehehrlich added inline comments to D55352: [elfabi] Introduce tool for ELF TextAPI.
Dec 11 2018, 5:58 PM

Dec 10 2018

jakehehrlich accepted D55533: [TextAPI][elfabi] Make SoName optional.

LGTM.

Dec 10 2018, 4:35 PM
jakehehrlich added inline comments to D55352: [elfabi] Introduce tool for ELF TextAPI.
Dec 10 2018, 4:19 PM

Dec 7 2018

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

Looks like a simple mechanical transformation. LGTM.

Dec 7 2018, 12:19 PM

Dec 6 2018

jakehehrlich added inline comments to D55352: [elfabi] Introduce tool for ELF TextAPI.
Dec 6 2018, 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.

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

Thanks! This looks good to me.

Dec 6 2018, 11:49 AM

Dec 5 2018

jakehehrlich committed rL348446: [llvm-objcopy] Change --only-keep to --only-section.
[llvm-objcopy] Change --only-keep to --only-section
Dec 5 2018, 6:07 PM
jakehehrlich closed D52180: [llvm-objcopy] Change --only-keep to --only-sections.
Dec 5 2018, 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)

Dec 5 2018, 4:02 PM

Dec 3 2018

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

Dec 3 2018, 2:46 PM
jakehehrlich added a comment to D54939: [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.

Dec 3 2018, 2:44 PM
jakehehrlich committed rL348174: [llvm-objcopy] Add --build-id-link-dir flag.
[llvm-objcopy] Add --build-id-link-dir flag
Dec 3 2018, 11:52 AM
jakehehrlich closed D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Dec 3 2018, 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.

Dec 3 2018, 11:46 AM

Nov 27 2018

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.

Nov 27 2018, 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.

Nov 27 2018, 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.

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

LGTM

Nov 27 2018, 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.

Nov 27 2018, 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.

Nov 27 2018, 2:55 PM · Restricted Project
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.

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

Responded to latest comments.

Nov 27 2018, 12:54 PM

Nov 26 2018

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

Nov 19 2018

jakehehrlich accepted D54741: [Driver] Link sanitizer runtime deps on Fuchsia when needed.
Nov 19 2018, 11:58 PM
jakehehrlich accepted D54727: [compiler-rt] Use zx_futex_wait_deprecated for Fuchsia sanitizer runtime.
Nov 19 2018, 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

Nov 19 2018, 1:58 PM · Restricted Project
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.

Nov 19 2018, 1:51 PM · Restricted Project

Nov 15 2018

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

Nov 14 2018

jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 14 2018, 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
Nov 14 2018, 5:36 PM

Nov 13 2018

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.

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

LGTM

Nov 13 2018, 9:30 AM

Nov 12 2018

jakehehrlich closed D54451: [libObject] Fix getDesc for Elf_Note_Impl.
Nov 12 2018, 7:17 PM
jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 12 2018, 6:37 PM
jakehehrlich updated the diff for D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 12 2018, 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.

Nov 12 2018, 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

Nov 12 2018, 5:57 PM
jakehehrlich committed rL346724: [libObject] Fix getDesc for Elf_Note_Impl.
[libObject] Fix getDesc for Elf_Note_Impl
Nov 12 2018, 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.
Nov 12 2018, 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.

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

Nov 10 2018

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.

Nov 10 2018, 7:07 PM
jakehehrlich added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 10 2018, 4:25 PM
jakehehrlich created D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Nov 10 2018, 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