This is an archive of the discontinued LLVM Phabricator instance.

[Support] sys::getProcessTriple should return a macOS triple using the system's version of macOS
ClosedPublic

Authored by arphaman on Jun 21 2017, 6:23 AM.

Details

Summary

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running. This patch ensures that for macOS, sys::getProcessTriple returns a triple with the system's macOS version.

The function sys::getDefaultTargetTriple does this version adjustment as well. However, it seems wrong to me (the default target OS might not be the host OS). I left it alone in this patch to avoid breaking things, but I think that the version adjustment in sys::getDefaultTargetTriple should check that the host OS is the same as the target OS before making the adjustment.

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though). I do have a commit in Clang r305678 (that I reverted for now) that depends on this change.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jun 21 2017, 6:23 AM
dexonsmith edited edge metadata.Jun 21 2017, 3:04 PM

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though).

I think we could add a lit test with REQUIRES: shell that only runs on macOS and uses sw_vers to pull out the system OS version. If any builder has a non-matching LLVM_HOST_TRIPLE, we have some test coverage.

arphaman updated this revision to Diff 103567.Jun 22 2017, 6:38 AM

Add a test

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though).

I think we could add a lit test with REQUIRES: shell that only runs on macOS and uses sw_vers to pull out the system OS version. If any builder has a non-matching LLVM_HOST_TRIPLE, we have some test coverage.

I've added a unittest that uses sw_vers.

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

Lang, do you know if it's reasonable to change the behaviour of sys::getProcessTriple?

lhames accepted this revision.Jul 6 2017, 2:20 PM

Sorry for the delayed reply.

LGTM (though see inline comment).

The getProcessTriple function is pretty fundamentally broken as it stands: it doesn't do the right thing when you're cross compiling (e.g. an ARM compiler built on x86 will claim that the process triple is x86_64-...). One day we should tackle that. This seems like a strict improvement in the mean time though.

  • Lang.
lib/Support/Unix/Host.inc
37–53 ↗(On Diff #103567)

Minor nitpick: This seems like it would be nicer as a function from a string to a string, rather than a mutating operation.

This revision is now accepted and ready to land.Jul 6 2017, 2:20 PM
arphaman marked an inline comment as done.Jul 7 2017, 2:53 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 14 2021, 8:03 AM
thakis added inline comments.
llvm/trunk/lib/Support/Unix/Host.inc
58

This is the function that clang calls to determine its default triple (here: http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/CompilerInvocation.cpp#3410)

LLVM_DEFAULT_TARGET_TRIPLE is what we tell people to set when they build cross compilers. It's what we use to make sure our compiler isn't dependent on the host OS version.

Changing this here to rewrite LLVM_DEFAULT_TARGET_TRIPLE to use the host OS version seems like the wrong thing to do, from the perspective of clang's default triple. Do you agree?

Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 8:03 AM
keith added a subscriber: keith.Feb 18 2022, 4:41 PM
lhames added inline comments.Feb 27 2022, 2:09 PM
llvm/trunk/lib/Support/Unix/Host.inc
58

I missed this at the time.

It looks like this patch only partially changed the behavior here: We were already modifying the OS version for -darwin triples but now we're doing it for -macos too.

The JIT doesn't use getDefaultTargetTriple, so I don't mind if we switch to a behavior that makes more sense to clang. The getProcessTriple function should still update the version though.