This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]