This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] support the ref directive for object generation.
ClosedPublic

Authored by Esme on Feb 19 2023, 5:59 PM.

Details

Summary

A R_REF relocation as a non-relocating reference is required to prevent garbage collection (by the binder) of the ref symbol in object generation.

Diff Detail

Event Timeline

Esme created this revision.Feb 19 2023, 5:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2023, 5:59 PM
Esme requested review of this revision.Feb 19 2023, 5:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2023, 5:59 PM
shchenz added inline comments.Mar 7 2023, 4:55 PM
llvm/lib/MC/MCXCOFFStreamer.cpp
88

This would be very strange directly calling to XCOFFObjectWriter and skipping the abstract MC layer. For example, with this implementation I think you can not get the MCFixups for the refs in the MCAssembler dumping?

Is it possible to just create a MCFixup here and use existing logic to handle the relocations? With this method I think you don't need the std::vector<std::pair<const MCSymbol *, const MCFragment *>> RefSymbolInfo in the XCOFFObjectWriter.cpp, the MCFixups should all be attached to the MCFragment of the MCSection.

llvm/test/CodeGen/PowerPC/pgo-ref-directive.ll
122

We also need a case that has R_REF relocation for .text section.

Esme updated this revision to Diff 503622.Mar 8 2023, 10:01 PM

Add a Fixup of kind FK_NONE to later record a relocation of type R_REF.

shchenz added inline comments.Mar 15 2023, 8:01 PM
llvm/lib/MC/MCXCOFFStreamer.cpp
89

Do you think reusing PPC fixup kind fixup_ppc_nofixup for AIX R_REF type relocation makes more sense?

llvm/test/CodeGen/PowerPC/pgo-ref-directive.ll
11

Can we add RUN lines for another two cases?

Esme added inline comments.Mar 16 2023, 1:46 AM
llvm/lib/MC/MCXCOFFStreamer.cpp
89

The fixup_ppc_nofixup is defined in llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h, and it looks inappropriate to include that in llvm/lib/MC?

shchenz added inline comments.Mar 16 2023, 7:33 PM
llvm/lib/MC/MCXCOFFStreamer.cpp
89

yes, I think you would need an interface in class MCAsmBackend and override this interface in class XCOFFPPCAsmBackend. FK_NONE is not a good choice, it is for relocation types that are defined in targets and bigger FirstLiteralRelocationKind(Although now PPC does not have any fixup kind bigger than FirstLiteralRelocationKind, but we really should not break this common design.). See PPCAsmBackend::getFixupKindInfo().

Esme updated this revision to Diff 506459.Mar 19 2023, 8:40 PM

Address comment -- use fixup_ppc_nofixup instead of FK_NONE to tie the ref symbol.

shchenz accepted this revision as: shchenz.Mar 21 2023, 7:33 PM

Looks good to me except one nit.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
280

nit: although we don't have user for other fixup kinds, for functionality complete, could we add other cases as well? This is what other targets do.

This revision is now accepted and ready to land.Mar 21 2023, 7:33 PM
Esme added inline comments.Mar 22 2023, 9:33 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
280

I don't really agree with adding other cases that are not used in this patch, because we will neither assurance its features for these cases nor test them.
I found that BFD_RELOC_* relocations are supported in other target, for example D118136, and if necessary, we can also support it for AIX, but it should not be in this patch. What do you think?

shchenz added inline comments.Mar 22 2023, 10:12 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
280

OK

This revision was landed with ongoing or failed builds.Mar 23 2023, 2:11 AM
This revision was automatically updated to reflect the committed changes.