Re: [PATCH v2] gas: support for .pushsection and .popsection pseudo ops for coff

Hi Johannes,

 Thank you for your submission.  I've left the GAS code pieces for Jan to 
sort out, since he's picked up this part already.  Here are a couple of my 
observations as to the remaining parts of the change.

 Please reword using the imperative mood both for the change heading and 
the description.

> In order to be able to compile the Linux kernel as a COFF
> object (and further on as a PE32 native executable) the
> implementation of those pseudo ops has been copiied from

s/copiied/copied/

> Also, a mini-test for pushsection/popsection has been added
> and checked if it succeeds (at least on the x86-64 Linux
> architecture).

 We don't record in the change description what regression-testing has 
been done; that is good to mention in the change discussion (or cover 
letter if applicable).

 I've regression-tested your change across 263 targets, several of which 
using COFF.  There are a couple of issues:

tic4x-coff  +FAIL: .pushsection and .popsection support
tic54x-coff  +FAIL: .pushsection and .popsection support
z80-coff  +FAIL: .pushsection and .popsection support
z8k-coff  +FAIL: .pushsection and .popsection support

Specifically (first error quoted only):

.../gas/testsuite/gas/coff/pushpop.s:1: Error: Unknown opcode `a'.
.../gas/testsuite/gas/coff/pushpop.s:1: Error: Invalid label '.section'

for the first two respectively, and:

regexp_diff match failure
regexp "^.*0000.*01030000.*$"
line   " 0000 0103                                 ..              "
regexp_diff match failure
regexp "^.*0000.*02000000.*$"
line   " 0000 02                                   .               "

for the latter two.

 Evidently the TIC4X/TIC54X dialects are pickier about code formatting, so 
please add the missing leading tabs.  Additionally these targets have 
multi-octet bytes, so even with formatting fixed the output does not match 
the dump patterns.  I think the patterns will best be made stricter on the 
offset and more relaxed on the data, e.g.:

Contents of section a:
 0000 01[0 ]*03.*
Contents of section b:
 0000 02.*

-- because all we want to verify is that the data items are correctly 
assigned to the respective sections.  The #pass statement does not seem to 
be needed or wanted, as there's no trailing output expected.

 I second Jan in that we want a complete feature rather than adding it 
piecemeal, especially given that it's a small piece of code, so please 
implement `.previous' as well (which is also historically older and goes 
back almost to the beginning of our ELF support), so that e.g. autoconf 
feature checks won't have to be added for individual pseudo-ops (and in 
principle given the timeline an existing check out there may actually 
infer `.previous' support from `.pushsection'/`.popsection' presence).

 Last but not least please update the manual accordingly and propose a 
gas/NEWS entry, since it's a new feature.

  Maciej