This is an archive of the discontinued LLVM Phabricator instance.

[AIX][MC][NFC] Explicitly set containing csects on XCOFF Symbols
ClosedPublic

Authored by sfertile on Aug 9 2019, 2:10 PM.

Details

Summary
  • Add a setter and getter for the containing csect of a symbol
  • In setting containing csect, don't set twice. Also assert if trying to set a different section.
  • Set containing csect explicitly for globals
  • Map the TOC Anchor symbol to it's containing csect
  • use getContainingCSect() to lookup csect rather than the fragment in XCOFFObjectWriter

Diff Detail

Event Timeline

daltenty created this revision.Aug 9 2019, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 2:10 PM
daltenty retitled this revision from [AIX][MC] Explicitly set containing csects on XCOFF Symbols to [AIX][MC][NFC] Explicitly set containing csects on XCOFF Symbols.Aug 9 2019, 2:19 PM
jasonliu added inline comments.Aug 12 2019, 9:27 AM
llvm/lib/MC/MCSymbolXCOFF.cpp
14 ↗(On Diff #214449)

Is it necessary to put these two functions in the .cpp file?
These two functions doesn't look complicated, and should be Okay to exist in the header.

sfertile accepted this revision.Aug 13 2019, 7:07 AM

LGTM. (After either responding to or addressing Jason comment)

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1688

minor nit: 'Csect' --> 'csect'

This revision is now accepted and ready to land.Aug 13 2019, 7:07 AM
hubert.reinterpretcast added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1688

Minor nits:

  • s/it's/its/;
  • Move "it" closer to its referent (the TOC anchor) by swapping the order of the first sentence: Since [ ... ], map [ ... ].
1689

That way, other methods can freely query it, assuming that it is set.

daltenty marked 2 inline comments as done.Aug 13 2019, 10:40 AM
daltenty added inline comments.
llvm/lib/MC/MCSymbolXCOFF.cpp
14 ↗(On Diff #214449)

I will move these to the header in the commit

sfertile commandeered this revision.Aug 21 2019, 8:36 AM
sfertile edited reviewers, added: daltenty; removed: sfertile.

Commandeering this.

This revision now requires review to proceed.Aug 21 2019, 8:36 AM
sfertile updated this revision to Diff 216412.Aug 21 2019, 8:41 AM
sfertile marked an inline comment as done.

We were stretching the definition of an NFC patch by creating but not using the TOC-base on AIX. I've striped this down to the NFC part and will post a separate patch for the TOC base change that includes a test change to reflect the TOC-base being created.

LGTM.

llvm/include/llvm/MC/MCSymbolXCOFF.h
40

s/contain/containing/;

This revision is now accepted and ready to land.Aug 21 2019, 9:11 AM
This revision was automatically updated to reflect the committed changes.