Re: cleanup PT_GNU_STACK size handling

[Mike Frysinger <vapier at gentoo dot org>]

On Tuesday 16 October 2012 10:45:37 Nathan Sidwell wrote:
> In working on a uclinux port, I came across the hacks that are in the
> current 5 uclinux backends of bfd (bfin, frv, lm32, sh, tic6x).

well, to be pedantic, FDPIC/ELF backends.  the format will work fine regardless 
of MMU support.

> The kernel relies on a non-zero memsz in a PT_GNU_STACK segment to know how
> much stack to allocate.

also to be clear, only the FDPIC/ELF loader needs memsz to be non-zero.  the 
ELF loader just checks the flags field.  so we're free to mess with the memsz 
field for all ELFs as no one has been looking at its size before.

> They all have the same set of overrides
> * modify_program_headers, munge the hdrs, updating a PT_GNU_STACK segment.
> * always_size_sections, looking for __stacksize variable and defaulting or
> setting it, if it is unset.
> * copy_private_data, propagating a PT_GNU_STACK segment size
> 
> That's quite a bunch of duplicated code, and I find the way of overriding
> the default via '--defsym __stacksize=VALUE' rather unpleasant.

i think it started with the FRV port and predated PT_GNU_STACK existence.  
every port since just copy & pasted most of the FDPIC code including this 
rather than revisiting if there was a better way.  i'm super happy you cleaned 
this up finally :).

it also shows that someone sufficiently motivated could probably unify some of 
the FDPIC bits more ...

> Actually,
> it doesn't work, because such a defsym gets a type NOTYPE and the check in
> always_size_sections for  'h->type == STT_OBJECT' fails.  One ends up with
> a duplicate symbol error. I have no idea how long that's been broken.

hmm, it works on Blackfin:
$ bfin-linux-uclibc-gcc test.c
$ bfin-linux-uclibc-readelf -l a.out | grep GNU_STACK
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x20000 RW  0x8

$ bfin-linux-uclibc-gcc test.c -Wl,--defsym,__stacksize=0x200
$ bfin-linux-uclibc-readelf -l a.out | grep GNU_STACK
  GNU_STACK      0x000000 0x00000000 0x00000000 0x00000 0x00200 RW  0x8

> I tested this on i686-pc-linux-gnu and all the above mentioned uclinux
> backends. There was one progression (ld-sh/fdpic-stack-size, the
> above-mentioned problem with '--defsym __stacksize')  The new tests pass. 
> I manually checked that objcopy et all preserved PT_GNU_STACK memsz, but
> my dejagnu-fu was insufficient to automate that.

i'm surprised you didn't see SORT_BY_ALIGNMENT start passing.  this has failed 
for us for the longest time because of this symbol showing up in the nm 
output.  see:
http://sourceware.org/ml/binutils/2008-04/msg00426.html

> Comments? Ok?

be nice to use unidiff style patches in the future ;)

style wise, there's a bunch of places that have trailing whitespace.  might 
want to double check & clean that up.  elf-bfd.h and bfd/elf.c pop out wrt 
that.

> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> ...
> struct elf_link_hash_entry *h = 0;

do we encourage this vs using NULL ?

> +      /* The symbol has no type if specified on the command line  */

needs a period at the end of that sentence

> +        (const char *) NULL, FALSE,

do you really need to cast that ?

> +    }
> +  return TRUE;

i think there's supposed to be a new line before that return

> +       /* Use -1 for explicit no-stack, because zero means
> +          'default' */

does that need to be line wrapped ?  either way, end of comment needs fixing -- 
period and two spaces.
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.