Paths

Table of Contentst

-
head/lib/libc/gen/
-
lib/
-
libc/
-
gen/
5/5
tls.c

Rework alignment handling in __libc_allocate_tls() for Variant I of TLS layout.
ClosedPublic
Actions

Authored by mmel on Dec 5 2017, 4:46 PM.

Details

Reviewers

kib
brooks
mizhka
jhibbits
andrew
br

Commits

rS326794: Rework alignment handling in __libc_allocate_tls() for Variant I of TLS layout.

Summary

Rework alignment handling in __libc_allocate_tls() for Variant I of TLS layout.

There are two versions of variant I of TLS

ARM and aarch64 uses original version of variant I here TP points to start of TCB followed by aligned TLS segment. Both TCB and TLS must be aligned to alignment of TLS section. The TCB[0] points to DTV vector and DTV values are real addresses (without bias).

MIPS, PowerPC and RISC-V uses modified, incomatible version of variant I, where TP points (with bias) to TLS and TCB immediately precedes TLS without any alignment gap[4]. Only TLS should be aligned. The TCB[0] points to DTV vector and DTV values are biased by constant value (0x8000) from real addresses.

Take this in account when allocating memory for TLS structures.

MFC after: 1 month

There are at least one oustanding issue: I cannot find any evidence
if a DTV vector is used in Local Exec TLS Model and if so, whether
is expected that it contains biased TLS values (addresses).

Test Plan

I made a simple test, so it would be nice if you can run it on the
affected platforms: https://people.freebsd.org/~mmel/test_tls.tgz
Thanks.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mmel created this revision.Dec 5 2017, 4:46 PM

Herald added a subscriber: imp. · View Herald TranscriptDec 5 2017, 4:46 PM

mizhka mentioned this in D13134: [mips32/tls] change TCB size from 8 to 16 to be aligned with r324938 & r325364.Dec 5 2017, 7:19 PM

landonf added a subscriber: landonf.Dec 5 2017, 7:43 PM

MIPS is fine:

[RT-N53:/tmp]$ ./test_tls 
OK.

Tested on Asus RT-N53 with jemalloc correction. jemalloc issue (incorrect debug assertion) has been discussed with David Goldblatt in https://gitter.im/jemalloc/jemalloc. It's accepted as bug to be fixed before 5.1 planned for early next year.

This revision is now accepted and ready to land.Dec 5 2017, 9:21 PM

I did not found anything obviously wrong (i.e. an easy to see contradiction between the comment and code).

lib/libc/gen/tls.c
143 ↗	(On Diff #36245)	Perhaps commit the malloc_aligned() changes separately. Am I right that this is needed only for Cheri ?
172 ↗	(On Diff #36245)	use
205 ↗	(On Diff #36245)	Why int and not size_t ? Also, order the variables in declaration alphabetically.
207 ↗	(On Diff #36245)	fragments
240 ↗	(On Diff #36245)	the following
261 ↗	(On Diff #36245)	Same note for the type and ordering.
269 ↗	(On Diff #36245)	fragments
287 ↗	(On Diff #36245)	Space between Elf_addr and '**'.

This code generally seem like an improvement for code where there are hidden alignment requirements in the binary, but it isn't complete. If you do something like:

extern __thread char array_16k[16384];
__attribute__((__aligned__(16384)))
__thread char array_16k[16384];

Then it will only reliably be aligned to 8k. On at least mips64 this will be a regression.

Kib,
thanks. I will incorporate all your comments into final commit.

Brooks,
I'm sorry for my ignorance, but why? I don't see single reason for this. Can you be more verbose, please.

Closed by commit rS326794: Rework alignment handling in __libc_allocate_tls() for Variant I of TLS layout. (authored by mmel). · Explain WhyDec 12 2017, 11:25 AM

This revision was automatically updated to reflect the committed changes.

First the overall alignment is computed:

maxalign = MAX(tcbalign, tls_init_align);

and then tls_block_size is computed with all the assorted padding and without relation to tcbalign.
The whole allocation is then aligned to maxalign with:

tls_block = malloc_aligned(tls_block_size, maxalign);

However, if tcbalign is > tls_block_size the TLS segment will be aligned to tls_block_size not maxalign. As a practical matter I'm not sure this can happen, but this code is confusing enough that I really don't want to have to read through multiple codepaths and traverser weak symbols to convince myself it won't. We should either honor all reasonable values for tcbalign is document our assumptions about them clearly.

Revision Contents
Changeset List

Path

Size

head/

lib/

libc/

gen/

tls.c

138 lines

Diff 36499

View Options

head/lib/libc/gen/tls.c

Show First 20 Lines • Show All 190 Lines • ▼ Show 20 Lines	main(int argc, char *argv[])
}		}

compile();		compile();

/* Continue with first and start second usage */		/* Continue with first and start second usage */
if (*argv)		if (*argv)
for (; *argv; argv++)		for (; *argv; argv++)
add_file(*argv);		add_file(*argv);
else if (fflagstdin)		else if (fflagstdin)
		0mpAuthorUnsubmitted Done Inline Actions There has to be at least one input file specified. 0mp: There has to be at least one input file specified.
exit(rval);		exit(rval);
else		else
add_file(NULL);		add_file(NULL);
process();		process();
cfclose(prog, NULL);		cfclose(prog, NULL);
if (fclose(stdout))		if (fclose(stdout))
err(1, "stdout");		err(1, "stdout");
exit(rval);		exit(rval);
Show All 29 Lines	if (script == NULL) {
if (more != NULL)		if (more != NULL)
*more = 0;		*more = 0;
return (NULL);		return (NULL);
}		}
linenum = 0;		linenum = 0;
switch (script->type) {		switch (script->type) {
case CU_FILE:		case CU_FILE:
if (strcmp(script->s, "-") == 0) {		if (strcmp(script->s, "-") == 0) {
if ((f = fdopen(0, "r")) == NULL)		f = stdin;
		0mpAuthorUnsubmitted Done Inline Actions I am not sure if it makes any difference if I try to avoid depending on the presence of `/dev/stdin` by using the 0th descriptor here instead. 0mp: I am not sure if it makes any difference if I try to avoid depending on the presence of…
err(1, "%s", "stdin");
fname = "stdin";		fname = "stdin";
		tobikUnsubmitted Done Inline Actions Can this not just be `f = stdin`? tobik: Can this not just be `f = stdin`?
		0mpAuthorUnsubmitted Done Inline Actions Good catch! The tests are still passing. 0mp: Good catch! The tests are still passing.
} else {		} else {
		0mpAuthorUnsubmitted Done Inline Actions Is `stdin` name here alright? Perhaps it could be changed to `-`? 0mp: Is `stdin` name here alright? Perhaps it could be changed to `-`?
if ((f = fopen(script->s, "r")) == NULL)		if ((f = fopen(script->s, "r")) == NULL)
err(1, "%s", script->s);		err(1, "%s", script->s);
fname = script->s;		fname = script->s;
}		}
state = ST_FILE;		state = ST_FILE;
goto again;		goto again;
case CU_STRING:		case CU_STRING:
if (((size_t)snprintf(string_ident,		if (((size_t)snprintf(string_ident,
▲ Show 20 Lines • Show All 302 Lines • Show Last 20 Lines

Rework alignment handling in __libc_allocate_tls() for Variant I of TLS layout.ClosedPublicActions