This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR gas/20312: Do not pad sections to alignment on failed assembly
- From: Alan Modra <amodra at gmail dot com>
- To: "Maciej W. Rozycki" <macro at imgtec dot com>
- Cc: binutils at sourceware dot org, Nick Clifton <nickc at redhat dot com>
- Date: Thu, 30 Jun 2016 11:29:48 +0930
- Subject: Re: [PATCH] PR gas/20312: Do not pad sections to alignment on failed assembly
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot DEB dot 2 dot 00 dot 1606290102220 dot 4103 at tp dot orcam dot me dot uk> <20160629015210 dot GN3665 at bubble dot grove dot modra dot org> <alpine dot DEB dot 2 dot 00 dot 1606291248470 dot 4103 at tp dot orcam dot me dot uk>
On Wed, Jun 29, 2016 at 04:26:40PM +0100, Maciej W. Rozycki wrote:
> On Wed, 29 Jun 2016, Alan Modra wrote:
>
> > > gas/
> > > PR gas/20312
> > > * write.c (subsegs_finish_section): Force no section padding to
> > > alignment on failed assembly, always set last frag's alignment
> > > from section.
> > > * testsuite/gas/all/pr20312.l: New list test.
> > > * testsuite/gas/all/pr20312.s: New test source.
> > > * testsuite/gas/all/gas.exp: Run the new test
> >
> > OK. Please also move the do_not_pad_sections_to_alignment test in
> > the write.c:SUB_SEGMENT_ALIGN definition to the use of
> > SUB_SEGMENT_ALIGN. There are a couple of targets that define their
> > own SUB_SEGMENT_ALIGN that would otherwise need modifying to hee
> > do_not_pad_sections_to_alignment.
>
> OK, but that does seem like material for a separate patch to me, right?
Yes, you're right. Please commit your original patch.
> For PR gas/20312 it is enough if `do_not_pad_sections_to_alignment' is
> set in `subsegs_finish_section' and then respected in `size_seg', and
> overall ISTM this further change you requested will be cosmetic. This is
> because any last frag padding produced in `subsegs_finish_section' will be
> ignored in `size_seg' anyway if `do_not_pad_sections_to_alignment' has
> been set.
>
> I've run the change below through regression testing against my usual set
> of targets and that scored no result changes at all, not even with ARM ELF
> targets (`arm-eabi', `arm-linuxeabi', `arm-netbsdelf') which are one of
> the two target groups affected, and not with the `elf/section11.d'
> ("Disabling section padding") test case in particular -- which seems to
> back up my understanding. The only other target group are SH COFF ones
> (covered by `sh-pe' in my testing), however these regrettably have no test
> suite coverage for the new feature as the test case mentioned is ELF only.
> All the remaining target-specific overrides wire SUB_SEGMENT_ALIGN to 0
> unconditionally anyway.
>
> So I think we might as well do nothing about it, and if we actually want
> to do something, e.g. to make it easier for the reader to understand what
> is going on here, then it should be enough if we only removed the
> `do_not_pad_sections_to_alignment' test from SUB_SEGMENT_ALIGN.
No, because just removing it will mean an alignment frag is added,
padding the section out according to previous .align/.p2align/.balign
directives. This padding affects the size *before* the
md_section_align affected by do_not_pad_sections_to_alignment in
size_seg.
> That said
> however I see no harm either from additionally strapping `alignment' to 0
> in `subsegs_finish_section' in the case affected.
>
> I'll push this change as a follow-up to the original fix then if you
> agree with my analysis.
My analysis yesterday agreed with you up to the point where you say we
could just remove do_not_pad_sections_to_alignment from
SUB_SEGMENT_ALIGN.
However, I've now looked a little more. In the ARM SUB_SEGMENT_ALIGN
there is a !(FRCHAIN)->frch_next term, and that means
recorded_alignment is ignored for padding at the end of a section.
The SH SUB_SEGMENT_ALIGN also ignores recorded_alignment. So the main
purpose of -no-pad-sections will be fulfilled, and the followup patch
I suggested isn't needed. Yes, a stray byte at the end of a text
section will pad the section out to an insn boundary, but that's
probably a good thing.
--
Alan Modra
Australia Development Lab, IBM