[PATCH] D61694: Boilerplate for producing XCOFF object files from the PowerPC backend.

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 22 inline comments as done.
sfertile added inline comments.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:30
+                                          MCSymbolAttr Attribute) {
+  llvm_unreachable("Not implemented yet");
+  return false;
----------------
hubert.reinterpretcast wrote:
> If the intent is to fail with assertions but silently continue in release, then this should be
> `assert(false && "Not implemented yet");`
> 
> Please review other uses of `llvm_unreachable` in this patch.
I've updated to report_fatal_error so we get the same behavior in release and debug builds, but just realized I left the dead 'return' in. I'll fix this in the next update.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:68
+  // Timestamp converted to an int32_t.
+  W.write<int32_t>(Now);
+  // Byte Offset to the start of the symbol table.
----------------
thakis wrote:
> hfinkel wrote:
> > sfertile wrote:
> > > hfinkel wrote:
> > > > Will the tools on AIX complain if this is 0? Our general policy is that the compiler's output should be deterministic/reproducible and this breaks that. Looks like D23934 was never finished, but this value should be tied to that (once it's finished).
> > > > 
> > > 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?
> > 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 )
Docs claim 0 is fine and I had no issue in limited testing so I've changed to always using '0' for now. We can look at adding an option if needed when the toolchain is more usable on AIX.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:87
+  else
+    MAI = new PPCXCOFFMCAsmInfo();
 
----------------
stefanp wrote:
> So, previously the default was ELF now it is XCOFF.
> This probably won't make much of a difference but ideally we should be spelling out what we support.
> 
> ```
> 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.
Updated this and the Streamer creation to keep the defaults the same. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61694/new/

https://reviews.llvm.org/D61694