NonSFI mode: Make tests/common/register_set.h PIC-friendly.

NonSFI mode: Make tests/common/register_set.h PIC-friendly

For x86-32 obtain address via GOT instead.

This is a necessary preparation to be able to compile via 
pnacl-clang for Non-SFI NaCl.
ARM version is already position-independent.

BUG= https://code.google.com/p/chromium/issues/detail?id=408879

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=13751

[Junichi Uekawa, 2014-09-15 01:17:45]

uekawa@chromium.org changed reviewers:
+ hamaji@chromium.org, hidehiko@chromium.org, mseaborn@chromium.org

[Junichi Uekawa, 2014-09-15 01:17:46]

Hi,

I've updated register_set.h so that it resolves using GOT so that it is more
PIC-friendly.

ptal

[Junichi Uekawa, 2014-09-15 03:43:08]

https://codereview.chromium.org/546043004/diff/20001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/20001/tests/common/register_se...
tests/common/register_set.h:354: ".word _GLOBAL_OFFSET_TABLE_-(1b + 8)\n" \
hmmm,  pnacl-clang claims it doesn't know ".word " directive 

/ssd/nacl2/native_client/toolchain/linux_x86/pnacl_newlib/bin/pnacl-clang
-std=gnu99 -arch arm --pnacl-allow-native --pnacl-allow-translate -std=gnu99
-Wstrict-prototypes -O2 -g -fomit-frame-pointer -fasynchronous-unwind-tables
-Wall -Wundef -fdiagnostics-show-option -pedantic -Werror -Wno-variadic-macros
-Wno-extra-semi -Wno-unused-private-field --pnacl-arm-bias -DNACL_BUILD_ARCH=arm
-DNACL_BUILD_SUBARCH=32 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1
-D_GNU_SOURCE=1 -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506
-D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0
-DNACL_ANDROID=0 -Iscons-out/nacl-arm-pnacl-clang/include -I/ssd/nacl2 -c -o
scons-out/nacl-arm-pnacl-clang/obj/tests/exception_test/exception_test.o
tests/exception_test/exception_test.c

[Junichi Uekawa, 2014-09-15 12:36:51]

https://codereview.chromium.org/546043004/diff/20001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/20001/tests/common/register_se...
tests/common/register_set.h:354: ".word _GLOBAL_OFFSET_TABLE_-(1b + 8)\n" \
changing to .long made this pass, but clang-3.4 doesn't like function(GOT)
expression.
clang-3.5 seems to allow it.



On 2014/09/15 03:43:08, Junichi Uekawa wrote:
> hmmm,  pnacl-clang claims it doesn't know ".word " directive 
> 
> /ssd/nacl2/native_client/toolchain/linux_x86/pnacl_newlib/bin/pnacl-clang
> -std=gnu99 -arch arm --pnacl-allow-native --pnacl-allow-translate -std=gnu99
> -Wstrict-prototypes -O2 -g -fomit-frame-pointer -fasynchronous-unwind-tables
> -Wall -Wundef -fdiagnostics-show-option -pedantic -Werror -Wno-variadic-macros
> -Wno-extra-semi -Wno-unused-private-field --pnacl-arm-bias
-DNACL_BUILD_ARCH=arm
> -DNACL_BUILD_SUBARCH=32 -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1
> -D_GNU_SOURCE=1 -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506
> -D_XOPEN_SOURCE=600 -DDYNAMIC_ANNOTATIONS_ENABLED=1
> -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_ -DNACL_WINDOWS=0 -DNACL_OSX=0
-DNACL_LINUX=0
> -DNACL_ANDROID=0 -Iscons-out/nacl-arm-pnacl-clang/include -I/ssd/nacl2 -c -o
> scons-out/nacl-arm-pnacl-clang/obj/tests/exception_test/exception_test.o
> tests/exception_test/exception_test.c                                         
 
>                                                             
>

[Mark Seaborn, 2014-09-15 17:15:21]

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_set.h
File tests/common/register_set.h (left):

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_se...
tests/common/register_set.h:338: "adr r0, " #def_func "\n" \
Note that "adr" is already position independent.

The only problem with it is that it has limited range.  That doesn't seem to
have been a problem so far.  Changing this to the non-range-limited version
would be good, but maybe we can postpone it if it depends on a newer version of
LLVM?  (JF is working on updating PNaCl to use LLVM 3.5.)

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_se...
tests/common/register_set.h:255: /* Obtain def_func from GOT to fill progg_ctr
with known value. */ \
Typo: "prog_ctr"

[Junichi Uekawa, 2014-09-16 01:37:53]

ptal

I've made it a x86-32-only change for now.

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_set.h
File tests/common/register_set.h (left):

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_se...
tests/common/register_set.h:338: "adr r0, " #def_func "\n" \
On 2014/09/15 17:15:21, Mark Seaborn wrote:
> Note that "adr" is already position independent.
> 
> The only problem with it is that it has limited range.  That doesn't seem to
> have been a problem so far.  Changing this to the non-range-limited version
> would be good, but maybe we can postpone it if it depends on a newer version
of
> LLVM?  (JF is working on updating PNaCl to use LLVM 3.5.)

I see, I missed the point that it was already position independent.
Sounds good.

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/40001/tests/common/register_se...
tests/common/register_set.h:255: /* Obtain def_func from GOT to fill progg_ctr
with known value. */ \
On 2014/09/15 17:15:21, Mark Seaborn wrote:
> Typo: "prog_ctr"

Done.

[hidehiko, 2014-09-16 16:34:00]

https://codereview.chromium.org/546043004/diff/80001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/80001/tests/common/register_se...
tests/common/register_set.h:243: "push $0 \n" /* Leave space for prog_ctr */ \
nit: how about align the spacing with the above line?

[Junichi Uekawa, 2014-09-16 20:57:32]

https://codereview.chromium.org/546043004/diff/80001/tests/common/register_set.h
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/80001/tests/common/register_se...
tests/common/register_set.h:243: "push $0 \n" /* Leave space for prog_ctr */ \
On 2014/09/16 16:34:00, hidehiko wrote:
> nit: how about align the spacing with the above line?

Done.

[hamaji, 2014-09-17 04:51:47]

lgtm

[Mark Seaborn, 2014-09-17 20:23:02]

LGTM

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
tests/common/register_set.h:243: "push $0\n" /* Leave space for prog_ctr */ \
Nit: Add space before "/*" to match line above

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
tests/common/register_set.h:258: "1: addl $_GLOBAL_OFFSET_TABLE_ + (1b - 0b),
%eax\n" \
BTW, a slightly shorter and simpler version is:

/* Fill out prog_ctr with known value. */
"call 0f\n" \
"0: popl %eax\n" \
"addl $" #def_func " - 0b, %eax\n" \
"movl %eax, 0x20(%esp)\n" \

The version you have is a little more general because it would also work for a
function defined in a different DSO, so it makes sense to use your more general
version.

[Junichi Uekawa, 2014-09-17 23:09:41]

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
File tests/common/register_set.h (right):

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
tests/common/register_set.h:243: "push $0\n" /* Leave space for prog_ctr */ \
On 2014/09/17 20:23:02, Mark Seaborn wrote:
> Nit: Add space before "/*" to match line above

Done.

https://codereview.chromium.org/546043004/diff/100001/tests/common/register_s...
tests/common/register_set.h:258: "1: addl $_GLOBAL_OFFSET_TABLE_ + (1b - 0b),
%eax\n" \
On 2014/09/17 20:23:02, Mark Seaborn wrote:
> BTW, a slightly shorter and simpler version is:
> 
> /* Fill out prog_ctr with known value. */
> "call 0f\n" \
> "0: popl %eax\n" \
> "addl $" #def_func " - 0b, %eax\n" \
> "movl %eax, 0x20(%esp)\n" \
> 
> The version you have is a little more general because it would also work for a
> function defined in a different DSO, so it makes sense to use your more
general
> version.

Acknowledged.

[Junichi Uekawa, 2014-09-17 23:09:49]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-09-17 23:10:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/546043004/120001

[commit-bot: I haz the power, 2014-09-18 01:17:31]

Committed patchset #7 (id:120001) as 13751