-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Metal-produced universal binaries #38
Conversation
Overall, this looks quite good to me. Regarding making this consistent across all platforms, we could change the API to always return a collection of handles, and on ELF/COFF we just return a single-element vector, but on MachO we sometimes return multiple element vectors of handles. That way we can continue to build workflows that are truly agnostic to the underlying object file type. With regards to breaking API, that's totally fine with me. I say do whatever you need to to make the package better, let's bump the major version number be done with it. |
Maybe the test libraries could be lazy artifacts downloaded during the tests (I warmly recommend using |
What would we gain by that? The binaries are tiny; doesn't seem worth the overhead of managing artifacts. |
Fine for me! I've implemented that change; every |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
- Coverage 71.25% 70.22% -1.04%
==========================================
Files 45 47 +2
Lines 1249 1313 +64
==========================================
+ Hits 890 922 +32
- Misses 359 391 +32
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Keno Fischer <[email protected]>
That seems bad. |
Agreed |
Field offsets are identical, so I guess it's just the padding at the end: julia> structinfo(T) = [(fieldoffset(T,i), fieldname(T,i), fieldtype(T,i)) for i = 1:fieldcount(T)];
julia> structinfo(ObjectFile.MachO.MachOSection64{MachOHandle{IOStream}})
11-element Vector{Tuple{UInt64, Symbol, DataType}}:
(0x0000000000000000, :sectname, fixed_string{UInt128})
(0x0000000000000010, :segname, fixed_string{UInt128})
(0x0000000000000020, :addr, UInt64)
(0x0000000000000028, :size, UInt64)
(0x0000000000000030, :offset, UInt32)
(0x0000000000000034, :align, UInt32)
(0x0000000000000038, :reloff, UInt32)
(0x000000000000003c, :nreloc, UInt32)
(0x0000000000000040, :flags, UInt32)
(0x0000000000000044, :reserved1, UInt32)
(0x0000000000000048, :reserved2, UInt32)
# vs
julia> structinfo(ObjectFile.MachO.MachOSection64{MachOHandle{IOStream}})
11-element Vector{Tuple{UInt32, Symbol, DataType}}:
(0x00000000, :sectname, fixed_string{UInt128})
(0x00000010, :segname, fixed_string{UInt128})
(0x00000020, :addr, UInt64)
(0x00000028, :size, UInt64)
(0x00000030, :offset, UInt32)
(0x00000034, :align, UInt32)
(0x00000038, :reloff, UInt32)
(0x0000003c, :nreloc, UInt32)
(0x00000040, :flags, UInt32)
(0x00000044, :reserved1, UInt32)
(0x00000048, :reserved2, UInt32) EDIT: oh, this should probably be using StructIO's EDIT2: actually, that gets it wrong too: julia> section_header_size(ohs[1])
80
julia> StructIO.packed_sizeof(section_header_type(ohs[1]))
80 EDIT3: was missing a struct section_64 {
char sectname[16]; /* name of this section */
char segname[16]; /* segment this section goes in */
uint64_t addr; /* memory address of this section */
uint64_t size; /* size in bytes of this section */
uint32_t offset; /* file offset of this section */
uint32_t align; /* section alignment (power of 2) */
uint32_t reloff; /* file offset of relocation entries */
uint32_t nreloc; /* number of relocation entries */
uint32_t flags; /* flags (section type and attributes) */
uint32_t reserved1; /* reserved (for offset or index) */
uint32_t reserved2; /* reserved (for count or sizeof) */
uint32_t reserved3; /* reserved */
}; |
Thanks so much, Tim! |
Is there anything else you want to put in this release, now that it's breaking? If not, could you tag a version so that I can depend on this? |
Sounds like we'd gain something (at least for users not running the tests): #40 🙃 |
This PR adds the necessary pieces of functionality and bug fixes I need to parse universal MachO binaries as produced by Apple's Metal compiler/driver, so that I can extract the native GPU code and disassemble it (for the purpose of implementing a
@code_native
in Metal.jl).In summary:
findfirst
change[1], so need to check that this doesn't break dependents)[1]: in fact, if we're breaking the interface anyway, maybe we should make it so that
findfirst
returns an index, and not an object. That's howfindfirst
generally works in Base.With all this in place, I process Metal universal binaries like such:
@staticfloat Please review, and let me know what this needs to get merged. I'd like to rely on this from Metal.jl as soon as possible :-)
Closes #37, closes #7