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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCXCOFFObjectWriter.h:39
+
+} // namespace llvm
+
----------------
`} // end namespace llvm`


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:30
+                                          MCSymbolAttr Attribute) {
+  llvm_unreachable("Not implemented yet");
+  return false;
----------------
sfertile wrote:
> 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.
Unreachable `return false;` still present.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:57
+  // We always emit a timestamp of 0 for reporducibility, so ensure incremental
+  // linking is not enabled.
+  if (Asm.isIncrementalLinkerCompatible())
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > How is an XCOFF timestamp associated with "incremental linking" (apparently a feature of `link.exe`)?
> `IncrementalLinkerCompatible` is a field in the generic MCAssembler and I don't see anything to indicate that its limited to Windows. The  fact that toolchains that target 'Link.exe' are the only one to enabled it currently doesn't mean that won't change in the future. I'd rather be pedantic and show that there might be some compatibility issues between the behavior we have chosen for XCOFF and the MCAssembler behavior.
The comment should put the concern into context. Something like "in case, like with Windows COFF, such a timestamp is incompatible with incremental linking of XCOFF".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61694