This is an archive of the discontinued LLVM Phabricator instance.

Add AIX Target/ToolChain to Clang Driver
AcceptedPublic

Authored by apaprocki on Mar 22 2016, 10:47 AM.

Details

Reviewers
majnemer
rsmith
Summary

This patch adds Clang driver support for the AIX platform. This allows Clang to be used for compiling code / checking for errors, but does not allow for building executables, as AIX uses XCOFF and not ELF.

After applying this patch and the underlying D18359:

$ clang -v
clang version 3.8.0 (tags/RELEASE_380/final)
Target: powerpc-ibm-aix7.1.0.0
Thread model: posix
InstalledDir: /tmp/llvm-3.8/bin
Found candidate GCC installation: /tmp/gcc-4.8/lib/gcc/powerpc-ibm-aix7.1.0.0/4.8.5
Selected GCC installation: /tmp/gcc-4.8/lib/gcc/powerpc-ibm-aix7.1.0.0/4.8.5
Candidate multilib: .;@maix32
Candidate multilib: ppc64;@maix64
Selected multilib: .;@maix32

Diff Detail

Event Timeline

apaprocki updated this revision to Diff 51300.Mar 22 2016, 10:47 AM
apaprocki retitled this revision from to Add AIX Target/ToolChain to Clang Driver.
apaprocki updated this object.
apaprocki added a reviewer: rsmith.
apaprocki added a subscriber: cfe-commits.

Hi , I find one mistake in the lib/Basic/Targets.cpp. On my AIX 7.1 machine /usr/include/sys/inttypes.h, 64 bits wchar_t is unsigned int, not signed int.

#ifndef _WCHAR_T
#define _WCHAR_T
#ifdef __64BIT__
typedef unsigned int    wchar_t;
#else
typedef unsigned short  wchar_t;
#endif
#endif /* _WCHAR_T */

So, the code should be

if (this->PointerWidth == 64) {
  this->WCharType = this->UnsignedInt;
} else {
  this->WCharType = this->UnsignedShort;
}
apaprocki updated this revision to Diff 64328.Jul 18 2016, 8:27 AM

Fixed wchar_t type.

(I also re-based the patch on top of trunk)

majnemer added inline comments.
lib/Basic/Targets.cpp
718

Are we really supposed to define this macro? Does GCC define this? I cannot find where it does so in the source.

apaprocki added inline comments.Jul 18 2016, 11:59 AM
lib/Basic/Targets.cpp
718

I defined both _ALL_SOURCE and _REENTRANT because the SolarisTargetInfo defines them (__EXTENSIONS__ is the Solaris equivalent of _ALL_SOURCE). If that is an oversight in the Solaris work, I'll remove _ALL_SOURCE here. GCC does not define it by default.

majnemer added inline comments.Jul 18 2016, 12:13 PM
lib/Basic/Targets.cpp
718

After a closer examination of the gcc sources, it looks like ALL_SOURCE is defined if gcc is being used for C++ code (via CPLUSPLUS_CPP_SPEC).

I don't see where _REENTRANT is defined for AIX though...

apaprocki added inline comments.Jul 18 2016, 1:36 PM
lib/Basic/Targets.cpp
718

On AIX, when gcc is passed -pthread, it defines _THREAD_SAFE. I think this should be changed to

if (Opts.POSIXThreads)
  Builder.defineMacro("_THREAD_SAFE");

On Solaris, gcc defines _REENTRANT when -pthread is passed. I think it is a separate bug (and I can file a separate revision) to put the Solaris use of it also on an Opts.POSIXThreads check.

apaprocki updated this revision to Diff 64376.Jul 18 2016, 1:41 PM

Could you please attach a diff with additional context?

lib/Driver/ToolChains.cpp
3717–3735

If I am not mistaken, this change seem unrelated.

apaprocki updated this revision to Diff 64518.Jul 19 2016, 10:33 AM

Increased context and removed accidental inclusion of Solaris change.

majnemer accepted this revision.Jul 19 2016, 10:51 AM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Jul 19 2016, 10:51 AM

I wan to point to some issues, because I am also doing AIX support based on your patch and I tested and verified.

Firstly, in the lib/Driver/ToolChains.cpp line 3756

case llvm::Triple::ppc64:
  addPathIfExists(D, getDriver().SysRoot + getDriver().Dir + "/../lib64", Paths);
  addPathIfExists(D, getDriver().SysRoot + "/usr/lib64", Paths);
  break;

In fact, in AIX there is no /usr/lib64, we only have /usr/lib and crt0.o for 32 bits, crt0_64.o for 64 bits. We also have crti.o / crti_64.o for C++.

Secondly, we should also define _AIX71 for AIX 7.1 in the lib/Basic/Targets.cpp.

Thirdly, the linker construct job has many issues (maybe because you have not supported code generation) in the lib/Driver/Tools.cpp, AIX has much difference with Solaris in fact.

we should code like this:

 if (Args.hasArg(options::OPT_shared)) {
   CmdArgs.push_back("-bM:SRE");
   CmdArgs.push_back("-bnoentry");
 }
 
 if (Args.hasArg(options::OPT_static)) {
   CmdArgs.push_back("-bnso");
   CmdArgs.push_back("-bI:/lib/syscalls.exp");
}

.......

I will try my best to complete linker work and so on based on your patch.

I notice that one thing, you define _THREAD_SAFE for posix thread, it is right. In AIX, we do not have _REENTRANT.

Lastly, you should be careful one thing: In AIX, when we use -pthread option, gcc standard headers directory and linker directory is changed. You should consider this. You can try gcc -pthread -v to see the output.

In collusion, this patch has some work to do in AIX from the view of my point.

@apaprocki, as mentioned in our recent RFC ( http://lists.llvm.org/pipermail/llvm-dev/2019-February/130175.html ), IBM is working on AIX support for Clang and LLVM. We would like to continue the work on this patch; however, as you may be aware, LLVM is undergoing relicensing ( https://llvm.org/docs/DeveloperPolicy.html#relicensing ). As such, we would like to know whether this patch is available under the terms of the Apache 2.0 License with LLVM exceptions. Thanks.

@hubert.reinterpretcast Yes, this patch is available under the new license.

@hubert.reinterpretcast Yes, this patch is available under the new license.

Thank you, @apaprocki. We will be splitting this into updated and more granular patches.

D59048 has been posted with some initial OSTargetInfo changes. Noticeable differences include not defining _ALL_SOURCE and focus on 64-bit long double. This is consistent with the invocation provided with the recent v16.1 release of the IBM XL C/C++ compiler for AIX that incorporates Clang-based technology.

shafik added a subscriber: shafik.Sep 21 2023, 11:52 AM

@hubert.reinterpretcast is this still relevant? It looks like portions of this have landed, can we close it?

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 11:52 AM