Stubs out a number of the classes needed to produce a new object file format (XCOFF) for the powerpc-aix target. For testing input is an empty module which produces an object file with just a file header.
Just realized that this will change the AsmPrinter that will get created for triples like -mtriple=ppc32-- where no OS is specified. I'll fix this so Linux is still the default AsmPrinter.
I'm not super familiar with the MC layer but overall this looks good to me.
I have a couple of minor comments.
Also, it seems that there is a part of that patch that is missing. The test added: aix-xcoff-basic.ll causes an assert with top of trunk.
Why don't we pass in the is64Bit flag and the Triple into the function?
So, previously the default was ELF now it is XCOFF.
if (TheTriple.isOSDarwin()) MAI = new PPCMCAsmInfoDarwin(isPPC64, TheTriple); else if (TheTriple.isOSBinFormatELF()) MAI = new PPCELFMCAsmInfo(isPPC64, TheTriple); else if (TheTriple.isOSBinFormatXCOFF()) MAI = new PPCXCOFFMCAsmInfo(); else llvm_unreachable("Unknown binary format.");
However, I don't know if the above is safe. Some code may rely on having ELF as default. Either way, I don't think we can leave it as XCOFF default.
Here too maybe we want to keep the 64 bit flag?
No a zero value should be fine. Is the goal the compiler always produces reproducible output by default, or just that we can enable reproducible output with an option?
Yeah, good idea. The first few patches for the Objectwriter are 32-bit only but we can simply report an error on 64-bit output for now.
Interesting question. I'd say by default, but I don't know that we've had a situation before where the platform default is to produce non-reproducible outputs at a low level (e.g., in the object-file format).
I recommend that we just set this to zero for now, and then have a separate discussion on how to change this later as necessary and/or desired.
If 0 works fine here, then go with that. But there's precedent for writing the current time into the output by default: clang-cl does that, since link.exe /incremental silently mislinks if that field is 0. So we do the safe thing by default and only write a 0 if the user explicitly passes /Brepro to clang-cl (which finally makes Asm.isIncrementalLinkerCompatible() return false here: http://llvm-cs.pcc.me.uk/lib/MC/WinCOFFObjectWriter.cpp#1057 )
This is missing the - between the filename and the description.
I don't think overriding a function just to call the direct base version of it is necessary.
The use of the Register*Streamer functions is associated with registering the appropriate streamer factory function for the object file format given a specific architecture in lib/Target/*/MCTargetDesc. If there are no classes derived from MCXCOFFStreamer, then I don't think we need this.
Refer to the previous comment regarding a missing -.
If the intent is to fail with assertions but silently continue in release, then this should be
Please review other uses of llvm_unreachable in this patch.
As a class defined in a .cpp file, this should probably go in an unnamed namespace.
The llvm:: here seems noisy and should be unnecessary here (and in a good number of cases in this file).
Does this need a TODO?
Use // end anonymous namespace.
Use = default.
Granted, this does not override a function just to clearly invoke the implementation in the direct base class, but explicitly skipping to an indirect base class with no comment is not better. Also, the direct base class does not override AsmPrinter::doFinalization anyway.