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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
---|---|---|
1665 | 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.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.h | ||
---|---|---|
41 | Why don't we pass in the is64Bit flag and the Triple into the function? | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp | ||
91 | 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. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h | ||
55 | Here too maybe we want to keep the 64 bit flag? |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
69 | 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? | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h | ||
55 | 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. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
69 | 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. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
69 | 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 ) |
llvm/include/llvm/MC/MCAsmInfoXCOFF.h | ||
---|---|---|
2 | This is missing the - between the filename and the description. | |
llvm/include/llvm/MC/MCXCOFFStreamer.h | ||
31 | I don't think overriding a function just to call the direct base version of it is necessary. | |
llvm/include/llvm/Support/TargetRegistry.h | ||
515 | 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. | |
llvm/lib/MC/MCAsmInfoXCOFF.cpp | ||
2 | Refer to the previous comment regarding a missing -. | |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
31 | 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. | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
20 | As a class defined in a .cpp file, this should probably go in an unnamed namespace. | |
24 | The llvm:: here seems noisy and should be unnecessary here (and in a good number of cases in this file). | |
44 | Does this need a TODO? | |
73 | int32_t | |
75 | uint16_t | |
77 | uint16_t | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
21 | Use // end anonymous namespace. | |
23 | Use = default. | |
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
1651 | 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. |
Addressed review comments:
- Fixed up several comments
- removed overridden functions that only called based class implementation.
- fixed up logic for creating an MCAsmInfo and Streamers so that Linux/ELF remains the default.
- Removed code for registering an XCOFFStreamer creation callback as no classes extend the XCOFF streamer.
- reviewed usage of assert/llvm_unreachable. Converted most to report_fatal_error
- Always use a timestamp of '0' for reproducible. Fatal error if incremental linking is enabled.
Added MCSectionXCOFF class so we can create and switch to the .text section. This was implemented by Jason Liu, and it was in the branch I did the boilerplate work on, but wasn't up streamed itself as it was not testable on its own.
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
---|---|---|
31 | 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. | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
69 | 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. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp | ||
91 | Updated this and the Streamer creation to keep the defaults the same. |
llvm/include/llvm/BinaryFormat/XCOFF.h | ||
---|---|---|
33 | Missing close paren. | |
36 | No period to be consistent with the other comments. | |
38 | Uppercase "table" to be consistent with the other comments. | |
llvm/include/llvm/MC/MCContext.h | ||
254 | Why unsigned here, and XCOFF::StorageMappingClass elsewhere? | |
260 | I suggest trying: return std::tie(SectionName, MappingClass) < std::tie(Other.SectionName, Other.MappingClass); | |
llvm/include/llvm/MC/MCXCOFFObjectWriter.h | ||
23 | Use override. | |
25 | Use override. | |
34 | Most instances in the containing directory have a newline before the closing brace and "end namespace llvm". |
llvm/lib/MC/MCSectionXCOFF.cpp | ||
---|---|---|
22 | Is there a test for this? | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
26 | The prevailing style in the containing directory is not to have both virtual and override on the same declaration. This comment applies to multiple instances in this file. | |
56 | Typo: s/reporducibility/reproducibility/; | |
57 | How is an XCOFF timestamp associated with "incremental linking" (apparently a feature of link.exe)? | |
86 | No period after "namespace". | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.cpp | ||
86 | s/is64Bit/Is64Bit/; | |
87 | "Little-endian" with the hyphen. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp | ||
245 | "TOC entries are not supported yet." | |
249 | Typos. Suggestion (if indeed the restriction is inherent): | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
20 | Remove the period. |
Address review comments round 2. - Various adjustments to various comments. - Changed MappingClass in XCOFFSectionKey to use the enum type instead of unsigned. - Removed 'virtual' from a declaration that is also marked 'override'. - Add an Is64Bit fileld to the MCXCOFFObjectTargetWriter. - Extended aix-xcoff-basic.ll to test switching to the '.text' section. - Cleaned up comparison of XCOFFSectionKey by using std::tie.
llvm/include/llvm/MC/MCContext.h | ||
---|---|---|
260 | Good suggestion, thanks. | |
llvm/lib/MC/MCSectionXCOFF.cpp | ||
22 | There is now :) | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
57 | 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. |
llvm/include/llvm/MC/MCXCOFFObjectWriter.h | ||
---|---|---|
40 | } // end namespace llvm | |
llvm/lib/MC/MCXCOFFStreamer.cpp | ||
31 | Unreachable return false; still present. | |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
57 | 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". |
llvm/lib/MC/MCObjectFileInfo.cpp | ||
---|---|---|
769 | This choice of csect name is not a fundamental property of the object file format or of the ABI. A comment with suitable justification should be added. I believe a good justification is that, on AIX, the __section__ attribute is not implemented by the IBM XL compiler, and the choice of using ".text" is compatible with GCC. | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp | ||
249 | Suggestion (to avoid implying that there are valid machine pseudo-ops for XCOFF, should someone encounter the message): "Machine pseudo-ops are invalid for XCOFF." | |
253 | Copy/paste error. Suggestion: "ABI-version pseudo-ops are invalid for XCOFF." | |
257 | Copy/paste error. Suggestion: "Local-entry pseudo-ops are invalid for XCOFF." | |
llvm/test/CodeGen/PowerPC/aix-xcoff-basic.ll | ||
8 | The other RUN lines do not have < %s with two consecutive spaces. | |
32 | I would prefer to have a comment to the effect that the csect need not be present, but we generate it anyway. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
67 | Typo: s/Finialize/Finalize/; | |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp | ||
232 | The starting indentation level does not match that of the preceding class. | |
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
161 | See previous comment about the starting indentation level. | |
llvm/test/CodeGen/PowerPC/aix-xcoff-basic.ll | ||
32 | Note that, at least in the current status of this patch, the assembly output and the object output from LLVM do not describe the same abstract XCOFF content. |
- Various comment tweaks and spelling fixes.
- Some indentation fixes and formatting changes.
- Removed unreachable return statement.
- Fixed error messages related to unsupported assembler pseudo ops.
- Added some comments to test clarifying why we are checking for .text csect, and that the binary and text outputs don't represent the same XCOFF content.
This broke the Windows LLDB bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/6493/steps/build/logs/stdio
Maybe someone can enlighten me as to why the build bots aren't tripping up on this one, but our group is running into this when we pull this commit from the upstream:
In MCStreamer.h, the declaration of the pure virtual EmitSymbolAttribute is:
virtual bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr AttrKind, StringRef AttrVal = StringRef()) = 0;
In this revision, in MCXCOFFStreamer.h, the declaration is:
bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
This is ill-formed, because the 'override' declaration hides the pure virtual member function. Apparently the clang-fast build bots are ok with this, but I'm not sure how. A similar code segment (Compiler Explorer) fails on every clang compiler I've tried.
Have you modified the signature of EmitSymbolAttribute in your downstream repo? I see
/// Add the given \p Attribute to \p Symbol. virtual bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) = 0;
in trunk (line 492) and no one has modified the file for months. A change like this would land with no merge conflicts but still be semantically broken if you have modified the signature. FWIW it bootstraps on trunk at the time I landed it, (and I double checked again right now).
Sorry about that, I believe @rnk landed a fix in https://reviews.llvm.org/rL365548, the buildbot turned green in the build that picked up that commit. Thanks Reid.
Oh dear... I apologize for that lapse of concentration. You are completely correct, someone had modified the signature in a prior commit, and I hadn't gone back far enough to see it.
Thank you for the response.
Missing close paren.