This is an archive of the discontinued LLVM Phabricator instance.

[lld][Hexagon] convert call x@GDPLT to call __tls_get_addr
ClosedPublic

Authored by sidneym on Feb 11 2020, 2:13 PM.

Details

Summary

In the General Dynamic model, TLS variables are referenced by :

r0 = add(r24, #x@GDGOT)    // R_HEX_GD_GOT...
call x@GDPLT                         // R_HEX_GD_PLT_B22_PCREL

The linker must rebind the call x@GDPLT to call __tls_get_addr in order to dereference the tls variable.
The method Hexagon uses is similar to 32-bit SPARC.

The thunking code was very useful since it modeled what I needed to do. I would have liked to have been able to add the symbol in HexagonTLSSymbolUpdate if it was found to be needed. When I tried that the dynamic symbol table entry was not correct.

Diff Detail

Event Timeline

sidneym created this revision.Feb 11 2020, 2:13 PM

The linker must rebind the call x@GDPLT to call __tls_get_addr in order to dereference the tls variable.

This is weird. Is it fixable in the psABI?

lld/ELF/Relocations.cpp
1978

//

1985

lld uses variableName

lld/ELF/Relocations.h
117

lld uses functionName

sidneym updated this revision to Diff 244180.Feb 12 2020, 10:06 AM

Update functionNames.

The original ABI, which is over 10 years old, may have used SPARC and 32-bit x86 as a reference:
0x00 leal x@tlsgd(,%ebx,1), %eax
0x07 call x@tlsgdplt

or:
0x00 sethi %hi(@dtlndx(x)), %o0
0x04 add %o0, %lo(@dtlndx(x)), %o0
0x08 add %l7, %o0, %o0
0x0c call x@TLSPLT

SPARC adds the __tls_get_addr symbol in the assembler when a VK_Sparc_TLS_GD_CALL is found. The same could be done for Hexagon (VK_Hexagon_GD_PLT), avoiding the need to create the symbol, but that would mean objects not built with the change would fail to link if using lld and that relocation was used. The symbol update would still be needed.

With the above in mind, I should probably check for the existence of __tls_get_addr prior to calling hexagonNeedsTLSSymbol.

sidneym updated this revision to Diff 244519.Feb 13 2020, 1:40 PM
sidneym marked 2 inline comments as done.

Update variable names.
make sure __tls_get_addr doesn't already exist before creating it

MaskRay added inline comments.Feb 13 2020, 3:59 PM
lld/ELF/Relocations.cpp
1988

excess parentheses can be dropped.

lld/ELF/Writer.cpp
1824

partitions[0]

lld/test/ELF/hexagon-tls-gd-xform.s
35

Use ## for comments.

38

# RELA_GDPLT

r0 = add(r24, #x@GDGOT)    // R_HEX_GD_GOT...
call x@GDPLT                         // R_HEX_GD_PLT_B22_PCREL

Align //

sidneym updated this revision to Diff 244696.Feb 14 2020, 9:59 AM

remove parens, ## comments, change partition index.

sidneym marked 5 inline comments as done.Feb 14 2020, 10:00 AM
bcain added a comment.Feb 25 2020, 7:45 AM

@MaskRay and @sidneym -- is this change ready to go or are there outstanding review items?

The method Hexagon uses is similar to 32-bit SPARC.

Can you explain how 32-bit SPARC do this? What about 64-bit SPARC?

lld/test/ELF/hexagon-tls-gd-xform.s
2

It can be on the same line.

The method Hexagon uses is similar to 32-bit SPARC.

Can you explain how 32-bit SPARC do this? What about 64-bit SPARC?

One method SPARC would have used can be found in older versions of binutils.
+ if (!sparc_tls_symbol)
+ sparc_tls_symbol = gen_rtx_SYMBOL_REF (Pmode, "__tls_get_addr");
See: https://gcc.gnu.org/ml/gcc-patches/2003-09/msg00456.html

The Introduction section of the Hexagon ABI doc refers to some of the resources used, http://docs.oracle.com/cd/E19253-01/817-1984/ -- "The call instruction uses the special syntax, x@TLSPLT. This call references the TLS variable and generates the R_SPARC_TLS_GD_CALL relocation. This relocation instructs the link-editor to bind the call to the __tls_get_addr() function, and associates the call instruction with the GD code sequence." Hexagon does the same.

The 64Bit ABI didn't do this, it just makes the call.

sidneym updated this revision to Diff 246769.Feb 26 2020, 9:42 AM

Merged 2 lines in testcase.

sidneym marked an inline comment as done.Feb 26 2020, 9:42 AM
sidneym updated this revision to Diff 247249.Feb 28 2020, 6:18 AM

Fix clang-format issues

sidneym updated this revision to Diff 247345.Feb 28 2020, 12:26 PM

fix formatting concerns from clang-tidy

bcain added a comment.Mar 5 2020, 3:22 PM

Ping @MaskRay and @sidneym -- anything left barring commit?

Many variable names are still not fixed.

lld/ELF/Relocations.cpp
1982

outputSections

1983

needTlsSymbol

1985

Not done. os isd

1996

outputSections

lld/test/ELF/hexagon-tls-gd-xform.s
39

Without a colon, these checks are not effective.

grimar added a subscriber: grimar.Mar 6 2020, 12:46 AM
grimar added inline comments.
lld/ELF/Relocations.cpp
1986

I wonder if you could do the following?

bool hexagonNeedsTLSSymbol(...) {
  for (InputSectionBase *s : inputSections)
    if (s->isLive())
      for (Relocation &Rel : Isec->relocations)
        if (Rel.sym->type == llvm::ELF::STT_TLS && Rel.expr == R_PLT_PC)
          return true;
  return false;
}
2003

I think it is better to move this find call out of the loops.

2005

How can this error happen?

sidneym marked an inline comment as done.Mar 6 2020, 7:08 AM
sidneym added inline comments.
lld/ELF/Relocations.cpp
2003

Yes, that would be better. I will do that.

Many variable names are still not fixed.

These had been changed per your original comment but changed back because a pre-checkin formatting pass flagged them as wrong. I made those name changes between diff set 6 and 7 (current).

Many variable names are still not fixed.

These had been changed per your original comment but changed back because a pre-checkin formatting pass flagged them as wrong. I made those name changes between diff set 6 and 7 (current).

https://github.com/google/llvm-premerge-checks/issues/142

You can just ignore the clang-tidy VariableName warnings.

sidneym updated this revision to Diff 249200.Mar 9 2020, 1:44 PM
  • Add the missing colon's that kept the part of the testcase from running.
  • Move the find of __tls_get_addr outside of the loop and return if it is not found.
  • Make __tls_get_addr preemptible. If isPreemptible is not set that symbol's symbol index is 0, since part of the testcase wasn't running, the missing colon's, I missed this.
  • Change the case of the variables.
MaskRay accepted this revision.Mar 12 2020, 5:59 PM
This revision is now accepted and ready to land.Mar 12 2020, 5:59 PM
bcain added a comment.Mar 12 2020, 6:16 PM

Ok, thanks. Now I think this is ready to go? LGTM at least.

sidneym updated this revision to Diff 250198.Mar 13 2020, 7:29 AM
sidneym marked an inline comment as done.Mar 13 2020, 8:16 AM
This revision was automatically updated to reflect the committed changes.