Message ID | 87inhu1ef5.fsf@esperi.org.uk |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
From: Nick Alcock <nick.alcock@oracle.com> Date: Fri, 11 Aug 2017 13:23:26 +0100 > unless you have a really serious linker bug, Open your mind a little bit. Maybe due to a bug in the Makefile the linker script doesn't get used, or the wrong one is used. Maybe parallel builds are not handled correctly. Narrowly focusing on the linker file directly could compromise your ability to diagnose this :-) I'll try to look at this myself soon, I've been too busy to do so lately. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Mon, 14 Aug 2017 22:35:53 -0700 (PDT) > From: Nick Alcock <nick.alcock@oracle.com> > Date: Fri, 11 Aug 2017 13:23:26 +0100 > >> unless you have a really serious linker bug, > > Open your mind a little bit. Looking at the actual details, how did this ever work for anyone? The arch/sparc/vdso/vdso.lds file says: vvar_start = . -(1 << 12); And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192. So, as requested, the object built gets: davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg fffffffffffff000 a vvar_data fffffffffffff000 a vvar_start So why is vdso2c requiring an 8192 byte aligned value for the address of 'vvar_start'? Nick, I will be honest and sat that I'm kinda irritated that I had to track this down and the best you could come up with was blaming the linker... :-( -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Sat, 09 Sep 2017 20:41:29 -0700 (PDT) > The arch/sparc/vdso/vdso.lds file says: > > vvar_start = . -(1 << 12); > > And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192. > > So, as requested, the object built gets: > > davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg > fffffffffffff000 a vvar_data > fffffffffffff000 a vvar_start Breaking this down further, the linker scripts are written assuming they will be built by a host compiler that generates 64-bit target code by default. This is not a safe assumption. My system generates 32-bit binaries from gcc by default, and that's why you end up with a value of PAGE_SIZE which is 4096 propagating through all of the generated linker scripts. x86, where it seems like were at least used as a template for the sparc linker scripts, gets away with this because the PAGE_SIZE is the same on 32-bit and 64-bit. Anyways, since 8192 is hard coded into vdso2c, just use 8192 as PAGE_SIZE in vdso-layout.lds.S and maybe put a comment above it. The next problem with the patch is that we get a warning because the variable 'secstrings_hdr' is set but not used in vdso2c.h, and compiler warnings are treated as errors under arch/sparc. This is fixed simply by: diff --git a/arch/sparc/vdso/vdso2c.h b/arch/sparc/vdso/vdso2c.h index 8d1b5cf..b193e3d 100644 --- a/arch/sparc/vdso/vdso2c.h +++ b/arch/sparc/vdso/vdso2c.h @@ -17,9 +17,8 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len, unsigned long mapping_size; int i; unsigned long j; - const char *secstrings; - ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr, *secstrings_hdr; + ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr; ELF(Ehdr) *hdr = (ELF(Ehdr) *)raw_addr; ELF(Dyn) *dyn = 0, *dyn_end = 0; INT_BITS syms[NSYMS] = {}; @@ -64,9 +63,6 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len, } /* Walk the section table */ - secstrings_hdr = raw_addr + GET_BE(&hdr->e_shoff) + - GET_BE(&hdr->e_shentsize)*GET_BE(&hdr->e_shstrndx); - secstrings = raw_addr + GET_BE(&secstrings_hdr->sh_offset); for (i = 0; i < GET_BE(&hdr->e_shnum); i++) { ELF(Shdr) *sh = raw_addr + GET_BE(&hdr->e_shoff) + GET_BE(&hdr->e_shentsize) * i; -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank you very much for this! On 10 Sep 2017, David Miller verbalised: > From: David Miller <davem@davemloft.net> > Date: Mon, 14 Aug 2017 22:35:53 -0700 (PDT) > >> From: Nick Alcock <nick.alcock@oracle.com> >> Date: Fri, 11 Aug 2017 13:23:26 +0100 >> >>> unless you have a really serious linker bug, >> >> Open your mind a little bit. > > Looking at the actual details, how did this ever work for anyone? Probably chance. It did seem to work, though. > The arch/sparc/vdso/vdso.lds file says: > > vvar_start = . -(1 << 12); > > And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192. Uh. Yeah. How the heck did I miss that? 50% chance it'll end up correctly aligned *anyway*, of course, and I must have been lucky. Blasted machine-variable page sizes. (You can tell this was derived from the x86 code :/ ) > So, as requested, the object built gets: > > davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg > fffffffffffff000 a vvar_data > fffffffffffff000 a vvar_start > > So why is vdso2c requiring an 8192 byte aligned value for the address > of 'vvar_start'? Because it has to be page-aligned. It's just, uh, the wrong alignment for this architecture. Mea culpa. > Nick, I will be honest and sat that I'm kinda irritated that I had to > track this down and the best you could come up with was blaming the > linker... :-( It's been two years since I looked at that code, and when Nagarathnam sent his email I was at the wrong end of a high-latency satellite link on holiday and then knee-deep in other time-critical stuff, I'm afraid. I entirely forgot this thread was still alive. On 11 Sep 2017, David Miller said: > Breaking this down further, the linker scripts are written assuming > they will be built by a host compiler that generates 64-bit target > code by default. > > This is not a safe assumption. One presumes it's not safe on x86-64 either, then. > My system generates 32-bit binaries from gcc by default, and that's > why you end up with a value of PAGE_SIZE which is 4096 propagating > through all of the generated linker scripts. *This* is down to distro variance, I'm afraid. (And yes, I think your machine is correctly configured, and I should have tested on a -m32-by-default GCC, but I didn't think of it.) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, I have fixed the issues with the patch and sent an updated patch. I was able to reproduce the issue in a qemu sparc emulator running Debian. I checked that the VDSO binaries pass compilation with the new patch in that environment. Thanks, Nagarathnam. On 9/10/2017 9:03 PM, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Sat, 09 Sep 2017 20:41:29 -0700 (PDT) > >> The arch/sparc/vdso/vdso.lds file says: >> >> vvar_start = . -(1 << 12); >> >> And "1 << 12" is 4096, therefore a multiple of 4096 and not 8192. >> >> So, as requested, the object built gets: >> >> davem@patience:~/src/GIT/sparc-next$ nm -n arch/sparc/vdso/vdso64.so.dbg >> fffffffffffff000 a vvar_data >> fffffffffffff000 a vvar_start > Breaking this down further, the linker scripts are written assuming > they will be built by a host compiler that generates 64-bit target > code by default. > > This is not a safe assumption. > > My system generates 32-bit binaries from gcc by default, and that's > why you end up with a value of PAGE_SIZE which is 4096 propagating > through all of the generated linker scripts. > > x86, where it seems like were at least used as a template for the > sparc linker scripts, gets away with this because the PAGE_SIZE > is the same on 32-bit and 64-bit. > > Anyways, since 8192 is hard coded into vdso2c, just use 8192 as > PAGE_SIZE in vdso-layout.lds.S and maybe put a comment above it. > > The next problem with the patch is that we get a warning because > the variable 'secstrings_hdr' is set but not used in vdso2c.h, and > compiler warnings are treated as errors under arch/sparc. > > This is fixed simply by: > > diff --git a/arch/sparc/vdso/vdso2c.h b/arch/sparc/vdso/vdso2c.h > index 8d1b5cf..b193e3d 100644 > --- a/arch/sparc/vdso/vdso2c.h > +++ b/arch/sparc/vdso/vdso2c.h > @@ -17,9 +17,8 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len, > unsigned long mapping_size; > int i; > unsigned long j; > - const char *secstrings; > > - ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr, *secstrings_hdr; > + ELF(Shdr) *symtab_hdr = NULL, *strtab_hdr; > ELF(Ehdr) *hdr = (ELF(Ehdr) *)raw_addr; > ELF(Dyn) *dyn = 0, *dyn_end = 0; > INT_BITS syms[NSYMS] = {}; > @@ -64,9 +63,6 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len, > } > > /* Walk the section table */ > - secstrings_hdr = raw_addr + GET_BE(&hdr->e_shoff) + > - GET_BE(&hdr->e_shentsize)*GET_BE(&hdr->e_shstrndx); > - secstrings = raw_addr + GET_BE(&secstrings_hdr->sh_offset); > for (i = 0; i < GET_BE(&hdr->e_shnum); i++) { > ELF(Shdr) *sh = raw_addr + GET_BE(&hdr->e_shoff) + > GET_BE(&hdr->e_shentsize) * i; -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- /dev/null +++ b/arch/sparc/vdso/vdso-layout.lds.S @@ -0,0 +1,106 @@ [...] +SECTIONS +{ + /* + * User/kernel shared data is before the vDSO. This may be a little + * uglier than putting it after the vDSO, but it avoids issues with + * non-allocatable things that dangle past the end of the PT_LOAD + * segment. + */ + + vvar_start = . -PAGE_SIZE; + vvar_data = vvar_start; + + . = SIZEOF_HEADERS; unless you have a really serious linker bug, there is no way vvar_start could fail to be at a multiple of 8192 bytes, AFAICS, and sym_vvar_start should have been set up by vdso2c to point right at it. The set but not used variable stuff suggests you've been doing stuff inside vdso2c, which I was just able to carry across unmodified from x86 and which worked fine when I did that. I bet some of that is somehow broken and the various symbol references in the vdso2c output are wrong somehow. -- NULL && (void) -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html