Re: [patch] General info in AS listings

["Santiago Urueña Pascual" <suruena at gmail dot com>]

Hi Nick,

>  Thanks very much for submitting this patch.  The first thing that I must
>  point out is that, as you anticipated, in order for us to be able to
>  accept patches from you, you need to complete a copyright assignment
>  with the FSF.  This must be done by you if the work is all your own, or
>  by your employer if the work was done for them (or using their
>  machines!).  I have attached the necessary form for you to fill out and
>  send off in order to start this process rolling.
>
OK, I'll send the form right now. (I knew that it would be probably
needed, but I'm too lazy with respect bureaucracy, and was just hoping
that in this case the patch would be small enough :-)

>  The next thing to consider is whether the feature is actually necessary
>  ?  For example it seems to me that all of the information generated by
>  your new option could also be obtained from the command line or via a
>  wrapper script.  So why add the option to the assembler ?  The point
>  here being that it is a bad idea to add unnecessary options - feature
>  bloat - because it just makes room for more bugs.
>
I understand your point.  It's true that currently all the info
printed by this patch can be gathered externally, but my idea was to
add more fields. For example, this feature is inspired by GCC's
-fverbose-asm switch:

$ cat hello.s
        .file   "hello.adb"
# GNU Ada version 4.1.2 20061220 for GNAT Pro 6.0.2 (20070620)
(i686-pc-linux-gnu)
#       compiled by GNU C version 4.1.2 20061220 for GNAT Pro 6.0.2 (20070620).
# GGC heuristics: --param ggc-min-expand=98 --param ggc-min-heapsize=128248
# options passed:  -fverbose-asm -mtune=pentiumpro
# options enabled:  -falign-loops -fargument-alias -fbranch-count-reg
# -fcommon -fearly-inlining -ffunction-cse -fgcse-lm -fident
# -finline-functions-called-once -fivopts -fkeep-static-consts
# -fleading-underscore -floop-optimize2 -fmath-errno -fpcc-struct-return
# -fpeephole -fsched-interblock -fsched-spec -fsched-stalled-insns-dep
# -fshow-column -fsplit-ivs-in-unroller -ftrapping-math -ftree-loop-im
# -ftree-loop-ivcanon -ftree-loop-optimize -ftree-vect-loop-version
# -fverbose-asm -m32 -m80387 -m96bit-long-double -maccumulate-outgoing-args
# -malign-stringops -mfancy-math-387 -mfp-ret-in-387 -mieee-fp
# -mno-red-zone -mpush-args -mtls-direct-seg-refs

.globl hello_E
        .data
        .type   hello_E, @object
        .size   hello_E, 1
hello_E:
...

So if this patch was accepted, my next e-mail would ask if gas has any
"default" options similar to the 'options enabled' gcc output to add
that field to this general info section. I suppose that some
architectures has some options that are enabled by default in each
version of gas, and that is more difficult to obtain using an external
tool. I know that the -a option currently implies -alhs, but it's more
interesting to know default options for the generated code.

Another important point is that this patch involves changes mainly to
the file listings.c, so no complexity is added to the code generation
and therefore there is not any danger in introducing instability or
maintainability problems to critical gas functionality.

>   One minor point is that when you are
>  adding a new feature, rather than just fixing a bug, you should also
>  include a patch to update the gas/NEWS file (or ld/NEWS file, etc).
>
>  Plus, the other part that is often forgotten when adding a new feature
>  is to add one or more tests to the testsuite to check that the feature
>  works and that it continues to work in the future.
>
OK, thanks for pointing me this out, and for all your comments about
the code. I'll modify the patch accordingly and resend the patch after
completing the copyright papers, of course if you think that this
feature should be merged in binutils.

Best regards,

-- 
Santiago Urueña Pascual <suruena@gmail.com>