This is an archive of the discontinued LLVM Phabricator instance.

[AIX][TLS] Generate .extern and .ref references to __tls_get_addr for local-exec accesses.
ClosedPublic

Authored by amyk on May 24 2023, 6:48 AM.

Details

Summary

Compiling with TLS variables requires -pthread, but if the user omits this option, the
compiler will not show any obvious indication during compilation that -pthread is
needed for programs using TLS variables. Instead, the user will experience a segmentation
fault when running programs with TLS variables in them and without specifying -pthread.

This patch aims to generate .extern/.ref references to __tls_get_addr[DS] for local-exec
accesses, in order to trigger an error from the linker to indicate that there is an undefined
symbol to __tls_get_addr. Doing so will remind the user to compile/link with -pthread.

Diff Detail

Event Timeline

amyk created this revision.May 24 2023, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 6:48 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
amyk requested review of this revision.May 24 2023, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 6:48 AM
amyk updated this revision to Diff 528838.Jun 6 2023, 6:48 AM

Rebased patch.

DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2822

I do not think it is efficient to put the code here. if there are a lot of thread local variables or the variables are reference in a lot of places, the code run enter as many time as the the number of thread local variable reference place.

and each time do the same thing

MCSymbol *TlsGetAddrDescriptor =
     createMCSymbolForTlsGetAddr(OutContext, XCOFF::XMC_DS);
 ExtSymSDNodeSymbols.insert(TlsGetAddrDescriptor);

can we do it in the function PPCAIXAsmPrinter::doFinalization()

for (auto &I : TOC) {
    // Add a single .ref reference to __tls_get_addr[DS] for the local-exec TLS
    // model on AIX. For TLS models that do not generate calls to TLS functions,
    // this .ref reference to __tls_get_addr helps generate a linker error to
    // an undefined symbol to __tls_get_addr, which indicates to the user that
    // compiling with -pthread is required for programs that use TLS variables.
    if (Subtarget->isPPC64() &&
        (I.first.second == MCSymbolRefExpr::VariantKind::VK_PPC_AIX_TLSLE)) {
      MCSymbol *TlsGetAddrDescriptor =
          createMCSymbolForTlsGetAddr(OutContext, XCOFF::XMC_DS);

      ExtSymSDNodeSymbols.insert(TlsGetAddrDescriptor);

      OutStreamer->emitXCOFFRefDirective(TlsGetAddrDescriptor);
      break;
    }
  }

for (MCSymbol *Sym : ExtSymSDNodeSymbols)
    OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);

it will run only once.

DiggerLin added inline comments.Jun 9 2023, 10:36 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2897

if there is not a __thread variable or there is not local exec mode, the loop will go over all the TC entries(the TC entries maybe a large number). if it is not efficient.

can we add a new data number "bool hasRefGetTLSAddr" for PPCAsmPrinter. ?

and set the "hasRefGetTLSAddr" in the lambda function
auto GetVKForMO = [&](const MachineOperand &MO) or other place ?

for example , change your code in the patch D149722 to

auto GetVKForMO = [&](const MachineOperand &MO) {
    // For TLS local-exec accesses on AIX, we have one TOC entry for the symbol
    // (with the variable offset), which is differentiated by MO_TPREL_FLAG.
    if (MO.getTargetFlags() & PPCII::MO_TPREL_FLAG) {
      // TODO: Update the query and the comment above to add a check for initial
      // exec when this TLS model is supported on AIX in the future, as both
      // local-exec and initial-exec can use MO_TPREL_FLAG.
      assert(MO.isGlobal() && "Only expecting a global MachineOperand here!\n");
      TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
      if (Model == TLSModel::LocalExec) {

        hasRefGetTLSAddr =true;

        return MCSymbolRefExpr::VariantKind::VK_PPC_AIX_TLSLE;
      }
      llvm_unreachable("Only expecting local-exec accesses!");
    }

and change the code here to

if (hasRefGetTLSAddr )  {
   MCSymbol *TlsGetAddrDescriptor =
          createMCSymbolForTlsGetAddr(OutContext, XCOFF::XMC_DS);

  ExtSymSDNodeSymbols.insert(TlsGetAddrDescriptor);
  OutStreamer->emitXCOFFRefDirective(TlsGetAddrDescriptor);
}

for (MCSymbol *Sym : ExtSymSDNodeSymbols)
    OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);
amyk updated this revision to Diff 531747.Jun 15 2023, 7:18 AM

Address Digger's review comments of adding a boolean HasRefGetTLSAddr to control whether or not to emit the reference.

amyk updated this revision to Diff 532523.EditedJun 18 2023, 9:06 PM

Rebase patch and update relocation tests after the test case updates in D149722 and D152669.

amyk updated this revision to Diff 534961.Jun 27 2023, 6:51 AM

Rebase patch.

amyk added a reviewer: Esme.Jul 12 2023, 1:14 PM
amyk added a comment.Jul 12 2023, 1:18 PM

Ping.
@DiggerLin @Esme Do any of you have any comments regarding this patch?

DiggerLin added inline comments.Jul 17 2023, 1:54 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
632

change to

SymName = SMC == XCOFF::XMC_DS  ? "__tls_get_addr" : .__tls_get_addr ;
2887

I know that any value of non PPC::GETtlsTpointer32AIX is OK here
but why it is PPC::LDtoc here ? can we use "PPC::GETtlsADDR" here ?

DiggerLin added inline comments.Jul 18 2023, 7:35 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2887

use PPC::GETtlsADDR64AIX maybe more specific.

amyk updated this revision to Diff 553491.Aug 25 2023, 8:47 AM
amyk marked 5 inline comments as done.

Rebase patch and address review comments from @DiggerLin.

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

Yeah, that's a good point. I just needed any other opcode and just chose PPC::LDtoc as that is the instruction that I was adding it to (since we have a load from the TOC in 64-bit local-exec).
That being said, using that opcode also makes sense to me since we're creating a reference to __tls_get_addr to begin with.

This revision is now accepted and ready to land.Aug 30 2023, 12:25 PM
This revision was landed with ongoing or failed builds.Sep 5 2023, 10:15 AM
This revision was automatically updated to reflect the committed changes.