This is an archive of the discontinued LLVM Phabricator instance.

Respect object format choice on Darwin
ClosedPublic

Authored by loladiro on Nov 9 2014, 1:28 PM.

Details

Summary

The object format can be set to something other than MachO, e.g.
to use ELF-on-Darwin for MCJIT. This already works on Windows, so
there's no reason it shouldn't on Darwin.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 15960.Nov 9 2014, 1:28 PM
loladiro retitled this revision from to Respect object format choice on Darwin.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: lhames.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: Unknown Object (MLST).

Hi,

Interesting idea. I could see that could be useful in reducing variance in a product if your expertise is in ELF. I'm sure you know, but you should be wary with this; I wouldn't bet on the backends being quite as object-format agnostic as you'd hope, quite apart from any interactions with externally defined objects.

I did spot one thing with the patch (see below). I also think Lang should have at least a brief glance at it. He's far more aware of whether this could work than me.

Cheers.

Tim.

lib/MC/MCObjectFileInfo.cpp
844–845 ↗(On Diff #15960)

The new condition is redundant: isOSBinFormatMachO is the same as your added clause, so it amounts to completely removing the isOSDarwin check. Which actually does seem like quite an improvement, even if the code you get out of x86_64-macosx-elf is clinically insane.

It actually works quite nicely in practice, but I agree that I won't enable it in production anytime soon. In the meantime, it's quite quite useful for testing purposes. I assume it only work so nicely because a lot of the work as already been done to support ELF-on-windows.

lib/MC/MCObjectFileInfo.cpp
844–845 ↗(On Diff #15960)

Ah, interesting. Does anybody know why the TT.isOSDarwin() was there in the first place?

t.p.northover added inline comments.Nov 9 2014, 6:12 PM
lib/MC/MCObjectFileInfo.cpp
844–845 ↗(On Diff #15960)

History. Until recently, the increasingly inaccurately named Triple only had 4 components. MachO was implied by either a Darwin OS or an explicit "MachO" environment/ABI.

Looks like a fairly mechanical change when the separate object format field was added, which didn't quite work in this case. This patch (well, the simplified version) is looking better and better!

lhames edited edge metadata.Nov 11 2014, 9:20 PM

Apologies for the delay. This looks good to me, but I'll get Jim Grosbach (who owns MC) to take a peek too - since this touches non-JIT codegen too.

loladiro updated this revision to Diff 17647.Dec 27 2014, 5:30 AM
loladiro added a reviewer: grosbach.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: grosbach.

Bumping with just the proposed change to remove TT.isOSDarwin(). Also putting @grosbach as the reviewer as suggested by @lhames.

grosbach edited edge metadata.Dec 27 2014, 2:57 PM

Glad to have a look. On holiday at the moment so if I don't get to it before then, please ping me on the 4th when I'm back at my desk where I have all the relevant bits available.
Jim

Is it possible to add a testcase? This is accessible from llvm-mc, no?

@grosbach. Sorry I forgot about this. Pinging now :). I would guess it's possible to add a testcase. What should it do? Just check that the filetype is ELF when setting the triple to say x86_64-apple-darwin14.0.0-elf?

Bump. This is a very simple thing and works well. @grosbach, were you waiting on me adding a test case, if so does the proposed one sounds ok?

loladiro updated this revision to Diff 24847.May 1 2015, 5:53 PM
loladiro edited edge metadata.

Updated with test case.

This revision was automatically updated to reflect the committed changes.