[PATCH v2] elf: Properly set sh_offset for .tbss sections

[H.J. Lu]

On Tue, Apr 29, 2025 at 2:44 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Tue, Apr 29, 2025 at 09:36:39AM +0800, H.J. Lu wrote:
> > On Tue, Apr 29, 2025 at 8:44 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Apr 29, 2025 at 8:05 AM Alan Modra <amodra@gmail.com> wrote:
> > > >
> > > > On Mon, Apr 28, 2025 at 08:08:56AM +0200, Jan Beulich wrote:
> > > > > On 28.04.2025 04:47, H.J. Lu wrote:
> > > > > > Set sh_offset for .tbss sections to their nominal offset after aligning.
> > > > > > They are not loaded from disk so the value doesn't really matter, except
> > > > > > when the .tbss section is the first one in a PT_TLS segment.  In that
> > > > > > case, it sets the p_offset for the PT_TLS segment, which according to
> > > > > > the ELF gABI ought to satisfy p_offset % p_align == p_vaddr % p_align.
> > > > > >
> > > > > > bfd/
> > > > > >
> > > > > > PR ld/32896
> > > > > > * elf.c (assign_file_positions_for_load_sections): Properly set
> > > > > > sh_offset for .tbss sections.
> > > > >
> > > > > Why does the order of checks (PT_LOAD then SHT_NOBITS) need changing here?
> > > > > While pre-existing, I'm also having a hard time seeing why among SHT_NOBITS
> > > > > sections SHF_TLS ones would need special casing here. While I agree that
> > > > > the usual requirements have to be met by PT_TLS, the entire change feels
> > > > > like it's papering over an issue elsewhere (presumably in the laying out of
> > > > > load segments).
> > > >
> > > > The patch is OK.  Please apply.
> > > >
> > > > Jan, there is a lot of history to this code.  sh_offset for SHT_NOBITS
> > > > is somewhat arbitrary since those sections are not loaded from the
> > > > image.  p_offset for a program header with p_filesz zero is likewise
> > > > arbitrary, except that the gABI makes no exception regarding alignment
> > > > modulo p_align for such headers.
> > > >
> > > > The problem is that various other tools break (or have been broken in
> > > > the past) when we make what seems like logical changes to our code,
> > > > such as the one you point out, that all SHT_NOBITS sections ought to
> > > > be treated the same here.  See pr12921 and pr25662.
> > > >
> > > > --
> > > > Alan Modra
> > >
> > > I'd like to avoid putting sh_offset of .tbss sections beyond EOF.
> > > I will see what I can do.
> > >
> >
> > I'd like to check in this v2 patch.   The difference is
> >
> > -   this_hdr->sh_offset = sec->filepos = off + adjust;
> > +   file_ptr aligned_off = off + adjust;
> > +   /* Put meaningless sh_offset of .tbss sections
> > +      without file contents somewhere within the first
> > +      page, in an attempt to not point past EOF and
> > +      avoid sh_offset of zero.  */
> > +   if (aligned_off >= (file_ptr) maxpagesize)
> > +     aligned_off = (aligned_off + align - 1) % align + 1;
> > +   this_hdr->sh_offset = sec->filepos = aligned_off;
> >
> > so that sh_offset of .tbss sections is  somewhere within the first page.
> >
> > OK for master?
>
> I'd rather you committed v1.  This one might make a TLS segment with
> both .tdata and .tbss look a little wierd.
>

I pushed v1.  I will keep the bug open and see if I can find a solution for
a TLS segment with  both .tdata and .tbss without looking weird.

-- 
H.J.