[0/3] binutils: read from stdin if input file is -

Message ID 20180213092559.56981-1-ahmad@a3f.at
Headers show
Series
  • binutils: read from stdin if input file is -
Related show

Message

Ahmad Fatoum Feb. 13, 2018, 9:25 a.m.
Changes behavior of dlltool, nlmconv, nm, objcopy, objdump and size
to allow niftiness like:

    printf "\xCC" | objdump -D -bbinary -mi386 -

Affects the layout of struct bfd as well. Is this ok?
Decided against adding an .is_stdin bit, because the thought of
inadvertently deleting files due to mismatched libbfd was unpleasant

Copyright assignment is in place. `make -C binutils check` runs through.

--

bfd/
	* bfd.c (struct bfd): Add optional temp_filename field that points
	at file to be removed when closing bfd.
	* bfd-in2.h: Regenerate.
        * opencls.c (bfd_open): read stdin if filename == NULL
binutils/
	* nm.c:      const-qualify read-only string parameters
    * nm.c:      read from stdin if input file is -
    * dlltool.c: read from stdin if input file is -
    * nlmconv.c: read from stdin if input file is -
    * objcopy.c: read from stdin if input file is -
    * objdump.c: read from stdin if input file is -
    * size.c:    read from stdin if input file is -
 

Ahmad Fatoum (3):
  bfd_fopen: read from stdin if filename == NULL
  nm: const-qualify read-only string parameters
  binutils: read from stdin if input file is -

 bfd/bfd-in2.h      |  3 +++
 bfd/bfd.c          |  3 +++
 bfd/opncls.c       | 66 ++++++++++++++++++++++++++++++++++++++++++++++++------
 binutils/dlltool.c |  9 +++++++-
 binutils/nlmconv.c |  5 ++++-
 binutils/nm.c      | 56 +++++++++++++++++++++++++--------------------
 binutils/objcopy.c | 38 ++++++++++++++++++++++++-------
 binutils/objdump.c | 10 +++++++--
 binutils/size.c    | 10 +++++++--
 9 files changed, 155 insertions(+), 45 deletions(-)

-- 
2.16.1

Comments

Nick Clifton Feb. 20, 2018, 2:31 p.m. | #1
Hi Ahmad,

> Changes behavior of dlltool, nlmconv, nm, objcopy, objdump and size

> to allow niftiness like:

> 

>     printf "\xCC" | objdump -D -bbinary -mi386 -

> 

> Affects the layout of struct bfd as well. Is this ok?


Yes, but I would like to request a couple of additions:

  * Please add a patch to the binutils/NEWS file to mention
    this new behaviour.

  * Please update the binutils/doc/binutils.texi file to
    document the new behaviour.

  * It would be really nice if you could create a new test
    (or two) in the binutils testsuite to verify that the
    new behaviour works.  I am not sure how difficult this
    will be however.

One other thing.  In patch 1:

+      XDELETEVEC ((char *) abfd->temp_filename);

I think it would be nicer to just use free() here, rather
than XDELETEVEC, since make_temp_name does just return a malloc'ed
buffer.  Speaking of which:

+          nbfd->temp_filename = make_temp_file (NULL);

You need to check to see if make_temp_file() returned NULL here...

Cheers
  Nick
Ahmad Fatoum Feb. 20, 2018, 2:55 p.m. | #2
> On 20Feb 2018, at 15:31, Nick Clifton <nickc@redhat.com> wrote:

> 

> Hi Ahmad,

> 

>> Changes behavior of dlltool, nlmconv, nm, objcopy, objdump and size

>> to allow niftiness like:

>> 

>>    printf "\xCC" | objdump -D -bbinary -mi386 -

>> 

>> Affects the layout of struct bfd as well. Is this ok?

> 

> Yes, but I would like to request a couple of additions:

> 

>  * Please add a patch to the binutils/NEWS file to mention

>    this new behaviour.

> 

>  * Please update the binutils/doc/binutils.texi file to

>    document the new behaviour.

Will do.

> 

>  * It would be really nice if you could create a new test

>    (or two) in the binutils testsuite to verify that the

>    new behaviour works.  I am not sure how difficult this

>    will be however.

Will see what I can do.

> 

> One other thing.  In patch 1:

> 

> +      XDELETEVEC ((char *) abfd->temp_filename);

> 

> I think it would be nicer to just use free() here, rather

> than XDELETEVEC, since make_temp_name does just return a malloc'ed

> buffer.

As make_temp_name uses XNEWVEC I thought it'd be safer to use its XDELETEVEC counterpart (which is, for now!, #defined as free),
but I can change it.

> Speaking of which:

> 

> +          nbfd->temp_filename = make_temp_file (NULL);

> 

> You need to check to see if make_temp_file() returned NULL here...

make_temp_file aborts the process (via xmalloc) if it fails to allocate, so NULL check is superfluous.

Cheers
Ahmad
Nick Clifton Feb. 20, 2018, 3:08 p.m. | #3
Hi Ahmad,

>> Speaking of which:

>>

>> +          nbfd->temp_filename = make_temp_file (NULL);

>>

>> You need to check to see if make_temp_file() returned NULL here...

> make_temp_file aborts the process (via xmalloc) if it fails to allocate, so NULL check is superfluous.


Ah - I was going by the comment in libiberty.h:

  /* Return a temporary file name or NULL if unable to create one.  */

  extern char *make_temp_file (const char *) ATTRIBUTE_MALLOC;

And the libiberty info documentation:

  -- Replacement: char* make_temp_file (const char *SUFFIX)

     Return a temporary file name (as a string) or 'NULL' if unable to
     create one.  SUFFIX is a suffix to append to the file name.  The
     string is 'malloc'ed, and the temporary file has been created.

I guess that the header file and documentation are out of date.

Cheers
  Nick