Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Tom Tromey <tromey@redhat.com>
To: Paul Pluzhnikov <ppluzhnikov@google.com>
Cc: Andreas Schwab <schwab@redhat.com>,
	Pedro Alves <pedro@codesourcery.com>,
	        gdb-patches@sourceware.org,
	Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: Re: [patch] Fix for PR gdb/10819
Date: Thu, 22 Oct 2009 17:44:00 -0000	[thread overview]
Message-ID: <m3d44fiask.fsf@fleche.redhat.com> (raw)
In-Reply-To: <8ac60eac0910220929v4cad21b4gcf706be716b13771@mail.gmail.com> 	(Paul Pluzhnikov's message of "Thu, 22 Oct 2009 09:29:52 -0700")

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> There is one more call to bsearch in solib-osf.c, but that file appears
Paul> to not be used anymore. Ok to deleted it?

It is referenced from config/alpha/alpha-osf3.mh, so I think it isn't
dead.

I would not worry about this call to bsearch.  It has been there since
revision 1.1 of that file, in 2001.  I think any problem it might
provoke probably would have been encountered by now.

Paul> +  if (cie_table->num_entries == 0)
Paul> +    {
Paul> +      gdb_assert (cie_table->entries == NULL);
Paul> +
Paul> +      /* The C standard (ISO/IEC 9899:TC2) requires the BASE argument to
Paul> +	 bsearch be a valid pointer even when the NMEMB argument is 0.
Paul> +
Paul> +	 Passing NULL for BASE and 0 for NMEMB is also known to cause
Paul> +	 Solaris-8 bsearch to call bsearch_cie_cmp with NULL ELEMENT
Paul> +	 (which doesn't expect that and crashes); see PR gdb/10819.
Paul> +
Paul> +	 Therefore, avoid calling bsearch under these conditions.  */

I'm ok with this paragraph but given that this is a C standard thing, it
could really just say something like "The C89 Standard requires BASE to
be non-NULL".

Paul> +  /* See comment in dwarf2-frame.c:find_cie on why this check
Paul> +     is necessary.  */

I'm not ok with this comment; references like this are fragile because
the referenced comment may change without anybody knowing to update this
one.

This patch is ok if you replace the second comment with something
self-contained.

thanks,
Tom


  reply	other threads:[~2009-10-22 17:44 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-10-22  4:48 Paul Pluzhnikov
2009-10-22  5:48 ` Jan Kratochvil
2009-10-22  6:14   ` Paul Pluzhnikov
2009-10-22 10:43     ` Pedro Alves
2009-10-22 11:09       ` Andreas Schwab
2009-10-22 15:34         ` Paul Pluzhnikov
2009-10-22 16:30           ` Paul Pluzhnikov
2009-10-22 17:44             ` Tom Tromey [this message]
2009-10-22 18:31               ` Paul Pluzhnikov
2009-10-22 20:13                 ` Tom Tromey
2009-10-22 20:46                   ` Paul Pluzhnikov
2009-10-22 18:11     ` Pedro Alves

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m3d44fiask.fsf@fleche.redhat.com \
    --to=tromey@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jan.kratochvil@redhat.com \
    --cc=pedro@codesourcery.com \
    --cc=ppluzhnikov@google.com \
    --cc=schwab@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox