Page MenuHomePhabricator

[PowerPC] Fix missing TLS symbol type.
ClosedPublic

Authored by stefanp on Aug 28 2020, 4:58 AM.

Details

Summary

Previous implementations for the TLS models General Dynamic and Initial Exec
were missing the ELF::STT_TLS type on symbols that required the type. This patch
adds the type.

Diff Detail

Event Timeline

stefanp created this revision.Aug 28 2020, 4:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 4:58 AM
stefanp requested review of this revision.Aug 28 2020, 4:58 AM

I'll not say this is a bug. For an undefined symbol, STT_TLS is not strictly required. For a definition, STT_TLS is indeed better.

I'll not say this is a bug. For an undefined symbol, STT_TLS is not strictly required. For a definition, STT_TLS is indeed better.

MaskRay,
Thank you for looking at this patch.

Sorry it took me a couple of days to reply to this. I wanted to clarify in my head what was going on.
The reason I added this patch was because I ran into this following setup:

clang -O3 -mcpu=pwr10 -fPIC -c PCRelTLS.c

The source file PCRelTLS.c has some examples of General Dynamic and has two TLS variables defined as:

extern __thread unsigned int x;
extern __thread unsigned int y;

Now, if I look at the symbol table I end up with:

7: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND x
8: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND y

However, when I link this object:

ld.lld --shared PCRelTLS.o -o PCRelTLS.so

It looks like LLD will enter the function

template <class ELFT>
static unsigned
handleTlsRelocation(RelType type, Symbol &sym, InputSectionBase &c,
                    typename ELFT::uint offset, int64_t addend, RelExpr expr) {
  if (!sym.isTls())
    return 0;

and then exit immediately and the relocations related to x and y won't be handled correctly.
So, I did what you see in this patch: I made sure that all of the symbols like x and y had the type STT_TLS.

Of course, there could be something I've missed in my logic.

MaskRay accepted this revision.Sep 1 2020, 1:59 PM

It is not well specified but I notice that GNU ld and/or gold support some TLS type relocations referencing STT_SECTION symbols. Supporting STT_SECTION can be difficult in LLD (https://bugs.llvm.org/show_bug.cgi?id=45600) and only provide minor size optimizations. Properly annotating symbols with STT_TLS is always nice - because STT_TLS symbols behave like in a different address space. An undefined STT_TLS can help with diagnostics.

This revision is now accepted and ready to land.Sep 1 2020, 1:59 PM
sfertile accepted this revision.Sep 1 2020, 5:06 PM

One minor nit, but otherwise LGTM.

llvm/lib/MC/MCELFStreamer.cpp
442

Real minor nit: Place this case with the other VK_PPC_GOT_TPREL cases.

stefanp updated this revision to Diff 289674.Sep 3 2020, 3:34 AM

Fixed nit and moved the position of the case in the switch.

This revision was landed with ongoing or failed builds.Sep 3 2020, 3:57 AM
This revision was automatically updated to reflect the committed changes.