This is an archive of the discontinued LLVM Phabricator instance.

Boilerplate for producing XCOFF object files from the PowerPC backend.
ClosedPublic

Authored by sfertile on May 8 2019, 1:01 PM.

Details

Summary

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.

Diff Detail

Event Timeline

sfertile created this revision.May 8 2019, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 1:01 PM
sfertile marked an inline comment as done.May 17 2019, 9:53 AM
sfertile added inline comments.
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?
I know that we don't intend to support 64 bit from the start but I assume we will want to support it at one point right? We can check the flag at the start and if it is 64 bit we can emit a fatal_error or unreachable as you have done in other places.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
91

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.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
55

Here too maybe we want to keep the 64 bit flag?

hfinkel added inline comments.May 28 2019, 8:54 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
69

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).

sfertile marked 2 inline comments as done.May 29 2019, 5:50 AM
sfertile added inline comments.
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.

hfinkel added inline comments.May 29 2019, 7:55 AM
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.

thakis added a subscriber: thakis.Jun 3 2019, 6:33 AM
thakis added inline comments.
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
assert(false && "Not implemented yet");

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.

sfertile updated this revision to Diff 206710.Jun 26 2019, 11:10 AM
sfertile marked an inline comment as not done.

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.

sfertile marked 22 inline comments as done.Jun 26 2019, 11:25 AM
sfertile added inline comments.
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):
"Invalid request to emit a machine pseudo-op for XCOFF."

llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
20

Remove the period.

sfertile updated this revision to Diff 206917.Jun 27 2019, 1:29 PM
sfertile marked 2 inline comments as done.
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.
sfertile marked 20 inline comments as done and an inline comment as not done.Jun 27 2019, 1:41 PM
sfertile added inline comments.
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.

sfertile updated this revision to Diff 208219.Jul 5 2019, 12:18 PM
sfertile marked 2 inline comments as done.
  • 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.

LGTM with minor comments.

llvm/lib/MC/MCObjectFileInfo.cpp
768

Minor nit: Add a comma after "[f]or example".

llvm/test/CodeGen/PowerPC/aix-xcoff-basic.ll
18

s/functionallity/functionality/;

36

s/llvm's defualt/LLVM's default/;

This revision is now accepted and ready to land.Jul 5 2019, 1:00 PM
This revision was automatically updated to reflect the committed changes.

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.

sfertile added a subscriber: rnk.Jul 9 2019, 2:59 PM

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.