public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simark@simark.ca>
To: Tom Tromey <tromey@adacore.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Fix comparator bug in cooked index
Date: Fri, 27 Jan 2023 23:20:10 -0500	[thread overview]
Message-ID: <25a578ac-5036-832a-3577-8eb27996157f@simark.ca> (raw)
In-Reply-To: <20230127195632.1570281-1-tromey@adacore.com>



On 1/27/23 14:56, Tom Tromey via Gdb-patches wrote:
> Simon pointed out that the cooked index template-matching patch
> introduced a failure in libstdc++ debug mode.  In particular, the new
> code violates the assumption of std::lower_bound and std::upper_bound
> that the range is sorted with respect to the comparison.
> 
> When I first debugged this, I thought the problem was unfixable as-is
> and that a second layer of filtering would have to be done.  However,
> on irc, Simon pointed out that it could perhaps be solved if the
> comparison function were assured that one operand always came from the
> index, with the other always being the search string.
> 
> This patch implements this idea.
> 
> First, a new mode is introduced: a sorting mode for
> cooked_index_entry::compare.  In this mode, strings are compared
> case-insensitively, but we're careful to always sort '<' before any
> other printable character.  This way, two names like "func" and
> "func<param>" will be sorted next to each other -- i.e., "func1" will
> not be seen between them.  This is important when searching.
> 
> Second, the compare function is changed to work in a strcmp-like way.
> This makes it easier to test and (IMO) understand.
> 
> Third, the compare function is modified so that in non-sorting modes,
> the index entry is always the first argument.  This allows consistency
> in compares.
> 
> I regression tested this in libstdc++ debug mode on x86-64 Fedora 36.
> It fixes the crash that Simon saw.

I'm glad to know this idea works.

Just some minor comments below, otherwise the patch LGTM.

> ---
>  gdb/dwarf2/cooked-index.c | 166 ++++++++++++++++++++------------------
>  gdb/dwarf2/cooked-index.h |  42 ++++++++--
>  2 files changed, 123 insertions(+), 85 deletions(-)
> 
> diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
> index 09b3fd70b26..f6b1df6e529 100644
> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -30,56 +30,38 @@
>  
>  /* See cooked-index.h.  */
>  
> -bool
> +int
>  cooked_index_entry::compare (const char *stra, const char *strb,
> -			     bool completing)
> +			     comparison_mode mode)
>  {
> -  /* If we've ever matched "<" in both strings, then we disable the
> -     special template parameter handling.  */
> -  bool seen_lt = false;
> +  /* We want to sort '<' before any other printable character.  So,
> +     rewrite '<' to something just before ' '.  */
> +#define MUNGE(c) (c == '<' ? '\x1f' : TOLOWER ((unsigned char) c))

Can you make it a local function, like

  auto munge = [] (char c)
    {
      ...
    };

?  Macros are harder to debug.

> +  /* Convenience aliases.  */
> +  const auto mode_compare = cooked_index_entry::COMPARE;
> +  const auto mode_sort = cooked_index_entry::SORT;
> +  const auto mode_complete = cooked_index_entry::COMPLETE;
> +
> +  SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd",
> +					   mode_compare) == 0);
> +  SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd",
> +					   mode_compare) == 0);

Maybe I'm missing something, but the two checks above seem identical.

> +  SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd",
> +					   mode_complete) == 0);
> +  SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd",
> +					   mode_complete) == 0);

And these two too.

> diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
> index 55eaf9955ab..1c291ba5694 100644
> --- a/gdb/dwarf2/cooked-index.h
> +++ b/gdb/dwarf2/cooked-index.h
> @@ -143,16 +143,46 @@ struct cooked_index_entry : public allocate_on_obstack
>       STORAGE.  */
>    const char *full_name (struct obstack *storage) const;
>  
> -  /* Compare two strings, case-insensitively.  Return true if STRA is
> -     less than STRB.  If one string has template parameters, but the
> -     other does not, then they are considered to be equal; so for
> -     example "t<x>" == "t<x>", "t<x>" < "t<y>", but "t" == "t<x>".  */
> -  static bool compare (const char *stra, const char *strb, bool completing);
> +  /* Comparison modes for the 'compare' function.  See the function
> +     for a description.  */
> +  enum comparison_mode

Just a nit, but I like to use enum class.  For the simple reason that
users do "comparison_mode::SORT" instead of just "SORT", and I find that
this little bit of context helps when reading.

> +  {
> +    COMPARE,
> +    SORT,
> +    COMPLETE,

It's slightly confusing to have a more called COMPARE, since the
function itself is called compare.  Perhaps SEARCH or MATCH?

Simon

  reply	other threads:[~2023-01-28  4:20 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-27 19:56 Tom Tromey
2023-01-28  4:20 ` Simon Marchi [this message]
2023-01-30 14:30   ` Tom Tromey
2023-01-30 10:33 ` Andrew Burgess
2023-01-30 15:15   ` Simon Marchi

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=25a578ac-5036-832a-3577-8eb27996157f@simark.ca \
    --to=simark@simark.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=tromey@adacore.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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).