Re: [PATCH] dwz.c: special-case SHT_NOBITS sections

[Mark Wielaard <mark@klomp.org>]

Hi Alexy,

I CCed jakub, who maintains dwz.c, in case he isn't on this list.

On Tue, 2018-02-06 at 14:50 +0300, Alexey Tourbin wrote:
> When eu-strip is used to create separate .debug files, and is invoked
> with the --strip-debug option instead of --strip-all, the .symtab
> section is kept in a.out, but eu-strip also creates an empty .symtab
> section in a.out.debug with type NOBITS, but with the original size.
>
>  Section Headers:
>    [Nr] Name     Type    Address          Off    Size   ES Flg Lk Inf Al
> -  [33] .symtab  SYMTAB  0000000000000000 002b10 0006a8 18     34  55  8
> -  [34] .strtab  STRTAB  0000000000000000 0031b8 0001d1 00      0   0  1
> +  [33] .symtab  NOBITS  0000000000000000 002110 0006a8 18     34  55  8
> +  [34] .strtab  NOBITS  0000000000000000 002110 0001d1 00      0   0  1
> 
> This makes dwz fail with the following message:
> dwz: Section offsets in a.out.debug not monotonically increasing
> 
> This is because dwz does not take into account the possibility of
> SHT_NOBITS sections, and always increments sh_offset by sh_size.
> 
> So one way to fix this would be to disable writing of empty sections
> in eu-strip.  However, this seems to be the expected behavior, which
> also matches the behavior of the strip program from binutils (regarding
> its "--only-keep-debug" option, the manpage reads: "the section headers
> of the stripped sections are preserved, including their sizes, but the
> contents of the section are discarded.")

Yes, I think this correct, or at least expected behavior.

> Thus the next best thing is to actually teach dwz about NOBITS
> sections
> (this is a subtle reference to the fact that dwz doesn't have a mailing
> list or a dedicated sourceware.org/bugzilla component, so dealing with
> dwz might be a bit harder in this respect.)
> 
> After the patch was first drafted, I came across a similar patch
> by Richard Guenther <rguenther@suse>.  He goes much further in that
> he tries to handle truly unordered sections by qsorting them first.
> Richard explains such binaries can be created by the kernel linker
> script.

Do you have a reference to this patch? Or could you post it?
Running dwz over kernel modules probably won't work at the moment. But
the more general behavior seems useful. There are probably other tools
that don't keep the section offsets monotonically increasing. The
elfutils tools do try to keep that behavior in general, but I don't
believe there is anything that really prevents reordering of section
offsets.

>   As I only need to use dwz with userland programs - moreover,
> I want to ensure that those programs meet certain criteria - I decided
> to proceed with this much simpler patch of my own that does not permit
> truly unordered sections.  More specifically, I only need to apply
> dwz to separate .debug files, and I expect it to keep their sections in
> the same order as in the original executable or shared object.  There
> are other differences: in Richard's patch, NOBITS sections are simply
> ignored, and their offsets are not updated, - in other words, they are
> left technically unordered in the output.
> 
> Here's a simple test case reproducible on Fedora 27 x86-64.

Thanks. I was wondering why it only shows up on Fedora 27.
The reason is that previously the .symtab and .strtab were the last
sections, after the .shstrtab section. But with F27 bith .symtab and
.strtab come before .shstrtab, which is now the last section.

So maybe another "workaround" might be to ignore the .shstrtab section
in dwz, because I assume dwz will rewrite it anyway.

>   The patch
> covers a larger set of cases by also handling allocatable NOBITS
> sections as well as non-allocatable.

It might be good to show a reproducer for that case too. Just to make
sure we fully understand why it happens.

> echo 'int main(){}' >test.c
> gcc -g -O2 test.c
> eu-strip --strip-debug -f a.out.debug
> dwz a.out.debug
> ---
>  dwz.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/dwz.c b/dwz.c
> index b3b779d..01bc553 100644
> --- a/dwz.c
> +++ b/dwz.c
> @@ -10141,7 +10141,8 @@ write_dso (DSO *dso, const char *file, struct
> stat *st)
>  	  for (j = 1; j < dso->ehdr.e_shnum; ++j)
>  	    if (dso->shdr[j].sh_offset < min_shoff && !last_shoff)
>  	      continue;
> -	    else if ((dso->shdr[j].sh_flags & SHF_ALLOC) != 0)
> +	    else if ((dso->shdr[j].sh_flags & SHF_ALLOC) != 0
> +		     && dso->shdr[j].sh_type != SHT_NOBITS)
>  	      {
>  		error (0, 0, "Allocatable section in %s after non-
> allocatable "
>  			     "ones", dso->filename);
> @@ -10157,7 +10158,9 @@ write_dso (DSO *dso, const char *file, struct
> stat *st)
>  	      {
>  		if (k == -1)
>  		  k = j;
> -		last_shoff = dso->shdr[j].sh_offset + dso-
> >shdr[j].sh_size;
> +		last_shoff = dso->shdr[j].sh_offset;
> +		if (dso->shdr[j].sh_type != SHT_NOBITS)
> +		  last_shoff += dso->shdr[j].sh_size;
>  	      }
>  	  last_shoff = min_shoff;
>  	  for (j = k; j <= dso->ehdr.e_shnum; ++j)
> @@ -10181,7 +10184,9 @@ write_dso (DSO *dso, const char *file, struct
> stat *st)
>  		dso->shdr[j].sh_offset
>  		  = (last_shoff + dso->shdr[j].sh_addralign - 1)
>  		    & ~(dso->shdr[j].sh_addralign - (GElf_Off) 1);
> -	      last_shoff = dso->shdr[j].sh_offset + dso-
> >shdr[j].sh_size;
> +	      last_shoff = dso->shdr[j].sh_offset;
> +	      if (dso->shdr[j].sh_type != SHT_NOBITS)
> +		last_shoff += dso->shdr[j].sh_size;
>  	      if (addsec != -1 && j == addsec)
>  		last_shoff += addsize;
>  	    }

The last two hunks look correct to me.

Cheers,

Mark