Re: [PATCH] PR gas/20312: Do not pad sections to alignment on failed assembly

["Maciej W. Rozycki" <macro at imgtec dot com>]

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)
 	{