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: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Alan Modra <amodra at gmail dot com>
- Cc: <binutils at sourceware dot org>, Nick Clifton <nickc at redhat dot com>
- Date: Wed, 29 Jun 2016 16:26:40 +0100
- 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>
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?
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. 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.
gas/
* write.c [HANDLE_ALIGN] (SUB_SEGMENT_ALIGN): Remove
`!do_not_pad_sections_to_alignment' check.
(subsegs_finish_section): Strap alignment to 0 if
`do_not_pad_sections_to_alignment'.
Maciej
binutils-gas-subsegs-finish-align-pad.diff
Index: binutils/gas/write.c
===================================================================
--- binutils.orig/gas/write.c 2016-06-29 15:38:46.552477172 +0100
+++ binutils/gas/write.c 2016-06-29 15:40:31.436743271 +0100
@@ -1712,7 +1712,6 @@ set_symtab (void)
code-bearing sections. */
#define SUB_SEGMENT_ALIGN(SEG, FRCHAIN) \
(!(FRCHAIN)->frch_next && subseg_text_p (SEG) \
- && !do_not_pad_sections_to_alignment \
? get_recorded_alignment (SEG) \
: 0)
#else
@@ -1742,7 +1741,8 @@ subsegs_finish_section (asection *s)
if (had_errors ())
do_not_pad_sections_to_alignment = 1;
- alignment = SUB_SEGMENT_ALIGN (now_seg, frchainP);
+ alignment = (do_not_pad_sections_to_alignment
+ ? 0 : SUB_SEGMENT_ALIGN (now_seg, frchainP));
if ((bfd_get_section_flags (now_seg->owner, now_seg) & SEC_MERGE)
&& now_seg->entsize)
{