Page MenuHomePhabricator

[LLD] Set alignment as part of Characteristics in TLS table.
ClosedPublic

Authored by luqmana on Oct 1 2020, 1:17 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=46473

LLD wasn't previously specifying any specific alignment in the TLS table's Characteristics field so the loader would just assume the default value (16 bytes). This works most of the time except if you have thread locals that want specific higher alignments (e.g. 32 as in the bug) *even* if they specify an alignment on the thread local. This change updates LLD to take the max alignment from tls section.

Diff Detail

Event Timeline

luqmana created this revision.Oct 1 2020, 1:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 1:17 AM
luqmana requested review of this revision.Oct 1 2020, 1:17 AM
luqmana edited the summary of this revision. (Show Details)Oct 1 2020, 1:28 AM
luqmana added a reviewer: ruiu.
luqmana edited the summary of this revision. (Show Details)Oct 1 2020, 3:13 AM
luqmana updated this revision to Diff 295556.Oct 1 2020, 6:45 AM
Fix style warnings.
luqmana updated this revision to Diff 296582.Oct 6 2020, 7:51 PM

Update tests.

luqmana removed a reviewer: grimar.Oct 7 2020, 2:38 AM
rnk added a comment.Oct 13 2020, 11:19 AM

Thanks for the fix!

llvm/include/llvm/Object/COFF.h
592

If there is already some non-zero alignment, you would have to mask it out, right? Maybe structure this as:

  • build the new alignment characteristic bits, 0 or some value
  • load, mask, or in, and store them in one operation

As this setter is currently used, this bug is probably not observable, but it's best to be safe.

luqmana updated this revision to Diff 298041.Oct 13 2020, 11:01 PM

Mask out existing alignment bits.

luqmana marked an inline comment as done.Oct 13 2020, 11:01 PM
rnk accepted this revision.Oct 14 2020, 9:22 AM

Looks good, suggestions optional.

lld/COFF/Writer.cpp
2054–2055

I have some optional nitty suggestions here. You could do something like this to return early and reduce indentation:

Defined *b = dyn_cast_or_null<Defined>(symtab->findUnderscore("_tls_used"));
if (!b)
  return;
...

Also, I'm not sure b is the most descriptive name. sym, tlsSym?

This revision is now accepted and ready to land.Oct 14 2020, 9:22 AM
luqmana updated this revision to Diff 298282.Oct 14 2020, 7:33 PM

Address nit: Use early return to reduce indentation.

luqmana marked an inline comment as done.Oct 14 2020, 7:34 PM
This revision was landed with ongoing or failed builds.Oct 14 2020, 7:37 PM
This revision was automatically updated to reflect the committed changes.
luqmana updated this revision to Diff 298316.Oct 15 2020, 12:26 AM

Accidentally messed up the last diff there. Update revision with final code change that was committed for visibility.