Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> On Mon, Nov 18, 2002 at 05:16:06PM -0500, Klee Dienes wrote:
> 
>>         * tic30-dis.c (print_par_insn): Use xstrdup instead of strdup.

Alan, to clarify something here.

I know BFD intentionally doesn't use the x*() functions.  Instead it 
tries to clean up and return an error indication when there is a 
malloc() failure.

What of the disassembler though?  GDB, which is depending on the 
disassembler, needs to be able to recover from low memory (aka malloc() 
failure) conditions.  Is using xstrdup() in the disassembler going to 
lead to memory leaks in the case where xmalloc() fails?

Andrew

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Tue, Nov 19, 2002 at 11:26:12AM -0500, Andrew Cagney wrote:
> I know BFD intentionally doesn't use the x*() functions.  Instead it 
> tries to clean up and return an error indication when there is a 
> malloc() failure.
> 
> What of the disassembler though?  GDB, which is depending on the 
> disassembler, needs to be able to recover from low memory (aka malloc() 
> failure) conditions.

I OK'd the patch too quickly, then remembered the no xmalloc rule..
Then on grepping through opcodes/*, I saw so many xmalloc and xstrdup
calls that I hardly felt like correcting the patch.  We're no worse
off with an xstrdup call than an unchecked strdup call.  :-(

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> On Tue, Nov 19, 2002 at 11:26:12AM -0500, Andrew Cagney wrote:
> 
>> I know BFD intentionally doesn't use the x*() functions.  Instead it 
>> tries to clean up and return an error indication when there is a 
>> malloc() failure.
>> 
>> What of the disassembler though?  GDB, which is depending on the 
>> disassembler, needs to be able to recover from low memory (aka malloc() 
>> failure) conditions.
> 
> 
> I OK'd the patch too quickly, then remembered the no xmalloc rule..
> Then on grepping through opcodes/*, I saw so many xmalloc and xstrdup
> calls that I hardly felt like correcting the patch.  We're no worse
> off with an xstrdup call than an unchecked strdup call.  :-(

Perhaphs the coding standard applies to just bfd?

Assuming it does apply to opcodes/, remember, you've got to start 
somewhere.  One good pace to start is to stop the addition of further 
stray xmalloc() calls.

Andrew

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Klee Dienes]

Another option would be to add a xmalloc_set_failure_handler() to 
libiberty for use by GDB.

Regardless of what we plan to do though, I argue that replacing 
unchecked malloc() and strdup() calls in opcodes/ with xmalloc and 
xstrdup is a useful step forward.  The real bug in the code is the lack 
of checking the return value; using malloc instead of xmalloc is just 
sweeping the problem under the rug.  Using xmalloc may not be the final 
solution, but at least it prevents random unknown crashes, and marks 
the location of the bug for a later "go through and fix all the xmalloc 
calls" pass.

I'm not arguing against a "no xmalloc in new code" rule, just that an 
unchecked xmalloc is better than an unchecked malloc.

On Tuesday, November 19, 2002, at 05:09 PM, Andrew Cagney wrote:

>> On Tue, Nov 19, 2002 at 11:26:12AM -0500, Andrew Cagney wrote:
>>> I know BFD intentionally doesn't use the x*() functions.  Instead it 
>>> tries to clean up and return an error indication when there is a 
>>> malloc() failure.
>>> What of the disassembler though?  GDB, which is depending on the 
>>> disassembler, needs to be able to recover from low memory (aka 
>>> malloc() failure) conditions.
>> I OK'd the patch too quickly, then remembered the no xmalloc rule..
>> Then on grepping through opcodes/*, I saw so many xmalloc and xstrdup
>> calls that I hardly felt like correcting the patch.  We're no worse
>> off with an xstrdup call than an unchecked strdup call.  :-(
>
> Perhaphs the coding standard applies to just bfd?
>
> Assuming it does apply to opcodes/, remember, you've got to start 
> somewhere.  One good pace to start is to stop the addition of further 
> stray xmalloc() calls.
>
> Andrew
>
>

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> Another option would be to add a xmalloc_set_failure_handler() to libiberty for use by GDB.

Just FYI, GDB is currently intercepting the calls by implementing its 
own xmalloc() and having them linked in before libiberty.  See utils.c.

> Regardless of what we plan to do though, I argue that replacing unchecked malloc() and strdup() calls in opcodes/ with xmalloc and xstrdup is a useful step forward.  The real bug in the code is the lack of checking the return value; using malloc instead of xmalloc is just sweeping the problem under the rug.  Using xmalloc may not be the final solution, but at least it prevents random unknown crashes, and marks the location of the bug for a later "go through and fix all the xmalloc calls" pass.

True.

Problem is, a malloc() -> xmalloc() transformation also sweeps the 
problem under the carpet.  The code no longer dumps core so it must be 
fixed, right?

Accepting a work around involves a trade off.  I think here, an implicit 
decision has already made: the disassembler shall use xmalloc(); the 
disassembler shall leak memory.

> I'm not arguing against a "no xmalloc in new code" rule, just that an unchecked xmalloc is better than an unchecked malloc. 

Which reminds me, how is the elimination of true/false from "bfd.h" going?

Andrew

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Wed, Nov 20, 2002 at 11:05:14AM -0500, Andrew Cagney wrote:
> Problem is, a malloc() -> xmalloc() transformation also sweeps the 
> problem under the carpet.  The code no longer dumps core so it must be 
> fixed, right?

Bombing with "out of memory" is nearly as bad.

> Accepting a work around involves a trade off.  I think here, an implicit 
> decision has already made: the disassembler shall use xmalloc(); the 
> disassembler shall leak memory.

The disassembler doesn't need to leak memory.  Hmm, I suppose it could
be difficult to free things that aren't exposed to the interface.  You'd
rather bfd_alloc, which is freed automagically on bfd_close, eh?

> >I'm not arguing against a "no xmalloc in new code" rule, just that an 
> >unchecked xmalloc is better than an unchecked malloc. 
> 
> Which reminds me, how is the elimination of true/false from "bfd.h" going?

Has that become my job?? ;)

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Klee Dienes]

Was the final decision from the earlier discussion as simple as:

typedef int bfd_boolean;

s/boolean/bfd_boolean
s/true/1
s/false/0

and deal with the fallout?  If so, I'm willing to do the conversion and 
post a patch.

(After all, it's been almost a week since I broke GAS for some 
processor I've never heard of.  What fun is that?)

On Wednesday, November 20, 2002, at 06:08 PM, Alan Modra wrote:

>>> I'm not arguing against a "no xmalloc in new code" rule, just that an
>>> unchecked xmalloc is better than an unchecked malloc.
>>
>> Which reminds me, how is the elimination of true/false from "bfd.h" 
>> going?
>
> Has that become my job?? ;)

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> Was the final decision from the earlier discussion as simple as:

I don't know if anyone came up with an implementation proposal, just 
that ``true'' and ``false'' and ``boolean'' should be removed from "bfd.h".

Andrew


> typedef int bfd_boolean;
> 
> s/boolean/bfd_boolean
> s/true/1
> s/false/0
> 
> and deal with the fallout?  If so, I'm willing to do the conversion and post a patch.

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Tue, Nov 26, 2002 at 05:03:48PM -0500, Andrew Cagney wrote:
> >Was the final decision from the earlier discussion as simple as:
> 
> I don't know if anyone came up with an implementation proposal, just 
> that ``true'' and ``false'' and ``boolean'' should be removed from "bfd.h".

Heh, Andrew isn't letting this die. :)

> >typedef int bfd_boolean;
> >
> >s/boolean/bfd_boolean
> >s/true/1
> >s/false/0
> >
> >and deal with the fallout?  If so, I'm willing to do the conversion and 
> >post a patch.

This sort of patch is probably best left to a global maintainer.
It's almost as much work to review such a patch (mainly checking that
true and false in comments are unchanged) as it is to generate.

Hmm, I'm inclined to just use "int" directly rather than introduce a
"bfd_boolean".  Unless I hear objections, that's what I'll do one of
these days..

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Ian Lance Taylor]

Alan Modra <amodra@bigpond.net.au> writes:

> Hmm, I'm inclined to just use "int" directly rather than introduce a
> "bfd_boolean".  Unless I hear objections, that's what I'll do one of
> these days..

I tend to think that bfd_boolean is better because it makes the code
slightly more self-documenting.  An int variable might hold any value,
but a bfd_boolean variable is clearly intended to hold only a true or
false value.

But I'm hardly fanatical about it.

Ian

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Tue, Nov 26, 2002 at 03:29:36PM -0800, Ian Lance Taylor wrote:
> Alan Modra <amodra@bigpond.net.au> writes:
> 
> > Hmm, I'm inclined to just use "int" directly rather than introduce a
> > "bfd_boolean".  Unless I hear objections, that's what I'll do one of
> > these days..
> 
> I tend to think that bfd_boolean is better because it makes the code
> slightly more self-documenting.  An int variable might hold any value,
> but a bfd_boolean variable is clearly intended to hold only a true or
> false value.

Yes, I agree that it's more self-documenting, but even better is to
use function names that are obviously predicates.  My reason for
disliking the typedef is that it hides the real type in the same
way that macros hide things.  When it comes to debugging code you
inevitably hit a situation where you need to ignore all the
documentation and look at all macros and typedefs to see what is
really going on.

> But I'm hardly fanatical about it.

Nor am I.  :)  So far, it's two people for "bfd_boolean", one for
"int".

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Klee Dienes]

Along those lines, I like bfd_boolean because it's a reminder that the 
routine uses (what I have always found to be) non-intuitive error-code 
semantics.  Without it, I would find myself typing

	if (bfd_do_something () != 0)
		bfd_perror ("hosed")

even more often than I do already.

Like Ian, I'm certainly not fanatical about the issue either.

On Tuesday, November 26, 2002, at 06:29 PM, Ian Lance Taylor wrote:

> Alan Modra <amodra@bigpond.net.au> writes:
>
>> Hmm, I'm inclined to just use "int" directly rather than introduce a
>> "bfd_boolean".  Unless I hear objections, that's what I'll do one of
>> these days..
>
> I tend to think that bfd_boolean is better because it makes the code
> slightly more self-documenting.  An int variable might hold any value,
> but a bfd_boolean variable is clearly intended to hold only a true or
> false value.
>
> But I'm hardly fanatical about it.
>
> Ian

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> On Tue, Nov 26, 2002 at 03:29:36PM -0800, Ian Lance Taylor wrote:
> 
>> Alan Modra <amodra@bigpond.net.au> writes:
>> 
> 
>> > Hmm, I'm inclined to just use "int" directly rather than introduce a
>> > "bfd_boolean".  Unless I hear objections, that's what I'll do one of
>> > these days..
> 
>> 
>> I tend to think that bfd_boolean is better because it makes the code
>> slightly more self-documenting.  An int variable might hold any value,
>> but a bfd_boolean variable is clearly intended to hold only a true or
>> false value.

In C, there is zero and non-zero.  There is no true or false.

What ever the function's type, its the avoidance of:

	if (bfd_foo_p() == TRUE)

that is important.

> Yes, I agree that it's more self-documenting, but even better is to
> use function names that are obviously predicates.  My reason for
> disliking the typedef is that it hides the real type in the same
> way that macros hide things.  When it comes to debugging code you
> inevitably hit a situation where you need to ignore all the
> documentation and look at all macros and typedefs to see what is
> really going on.
> 
> 
>> But I'm hardly fanatical about it.
> 
> 
> Nor am I.  :)  So far, it's two people for "bfd_boolean", one for
> "int".

Andrew

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[David O'Brien]

On Wed, Nov 20, 2002 at 11:05:14AM -0500, Andrew Cagney wrote:
> >Another option would be to add a xmalloc_set_failure_handler() to 
> >libiberty for use by GDB.
> 
> Just FYI, GDB is currently intercepting the calls by implementing its 
> own xmalloc() and having them linked in before libiberty.  See utils.c.

This has also caused us problems in FreeBSD'ville as we share libiberty
with binutils and gdb.  What about having a real handler that is
registered?  Even using a function pointer XMALLOC in which it's main()'s
job to set to what ever malloc is desired would help.

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Hans-Peter Nilsson]

On Wed, 27 Nov 2002, Alan Modra wrote:
> On Tue, Nov 26, 2002 at 03:29:36PM -0800, Ian Lance Taylor wrote:
> > I tend to think that bfd_boolean is better because it makes the code
> > slightly more self-documenting.  An int variable might hold any value,
> > but a bfd_boolean variable is clearly intended to hold only a true or
> > false value.

> > But I'm hardly fanatical about it.
>
> Nor am I.  :)  So far, it's two people for "bfd_boolean", one for
> "int".

One more for "int" here.  I agree that a boolean type has its
advantages in theory for clarity, but IMO the effects have now
proved to be a net negative, a maintenance burden.  Let's just
stick to "int".

brgds, H-P

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Ian Lance Taylor]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> On Wed, 27 Nov 2002, Alan Modra wrote:
> > On Tue, Nov 26, 2002 at 03:29:36PM -0800, Ian Lance Taylor wrote:
> > > I tend to think that bfd_boolean is better because it makes the code
> > > slightly more self-documenting.  An int variable might hold any value,
> > > but a bfd_boolean variable is clearly intended to hold only a true or
> > > false value.
> 
> > > But I'm hardly fanatical about it.
> >
> > Nor am I.  :)  So far, it's two people for "bfd_boolean", one for
> > "int".
> 
> One more for "int" here.  I agree that a boolean type has its
> advantages in theory for clarity, but IMO the effects have now
> proved to be a net negative, a maintenance burden.  Let's just
> stick to "int".

I don't agree with this argument.  We've had problems because
`boolean', `true', and `false' are widely used.  In fact, the comment
on those lines in bfd.h is and has been from the start:

/* I'm sure this is going to break something and someone is going to
   force me to change it.  */

We won't have problems with `bfd_boolean'.

Ian

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Wed, Nov 27, 2002 at 09:56:47PM -0800, Ian Lance Taylor wrote:
> We won't have problems with `bfd_boolean'.

OK, OK.  :)  "typedef int bfd_boolean" it is.  Plain 0 and 1 for values.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Andrew Cagney]

> On Wed, Nov 27, 2002 at 09:56:47PM -0800, Ian Lance Taylor wrote:
> 
>> We won't have problems with `bfd_boolean'.
> 
> 
> OK, OK.  :)  "typedef int bfd_boolean" it is.  Plain 0 and 1 for values.

You ment 0 and !0 right? :-^

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Alan Modra]

On Thu, Nov 28, 2002 at 06:23:42PM -0500, Andrew Cagney wrote:
> >OK, OK.  :)  "typedef int bfd_boolean" it is.  Plain 0 and 1 for values.
> 
> You ment 0 and !0 right? :-^

Actually, having started to do the edit, I'm having second thoughts
about using plain old 0 and 1.  I think I'll use FALSE and TRUE, and
#undef and #define these macros in bfd-in.h.  Besides being consistent
with a "boolean" type, it means less formatting changes.  Anyone
have any objections?


A whinge.  This style of function declaration is a pain:

boolean function_with_a_long_name_or_lots_of_params PARAMS ((type1 arg1,
                                                             type2 arg2,
                                                             typen argn));

This is much better:

boolean function_with_a_long_name_or_lots_of_params
  PARAMS ((type1 arg1, type2 arg2, typen argn));

The first style means more editing when changing the return type or
function name as all the following lines need re-indenting.

Same goes for definitions.  This is a pain:

int some_func (a, b, c)
     boolean                a;
     all_lined_up_sweetly * b;
     int                    c;

Writing code this way almost guarantees that over time your code will
end up looking like:

int some_func (a, b, c)
     bfd_boolean                a;
     not_lined_up * b;
     int                    c;

Ditto local vars.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Doug Evans]

Alan Modra writes:
 > Same goes for definitions.  This is a pain:
 > 
 > int some_func (a, b, c)
 >      boolean                a;
 >      all_lined_up_sweetly * b;
 >      int                    c;
 > 
 > Writing code this way almost guarantees that over time your code will
 > end up looking like:
 > 
 > int some_func (a, b, c)
 >      bfd_boolean                a;
 >      not_lined_up * b;
 >      int                    c;
 > 
 > Ditto local vars.

Amen!

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Ben Elliston]

>>>>> "Doug" == Doug Evans <dje@transmeta.com> writes:

  >> int some_func (a, b, c)
  >> boolean                a;
  >> all_lined_up_sweetly * b;
  >> int                    c;

  Doug> Amen!

Having things line up sweetly is what I call "studly gaps".  ;-)

Ben

Re: [RFA] Replace strdup with xstrdup in tic30-dis.c

[Doug Evans]

Andrew writes:
 > In C, there is zero and non-zero.  There is no true or false.
 > 
 > What ever the function's type, its the avoidance of:
 > 
 >      if (bfd_foo_p() == TRUE)
 > 
 > that is important.

s/There is no true or false./There is only false and !false./

I like bfd_boolean,
though recognizing the pitfalls of foo_p() == TRUE is important.

What if one didn't define TRUE and instead defined CANONICAL_TRUE?