Re: [RFA] c++/11734

[Doug Evans <dje at google dot com>]

On Thu, Jun 24, 2010 at 1:00 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> There may be more going on here though, I don't understand why
> Doug> decode_line_1 has *both* is_quoted and is_quote_enclosed (is there
> Doug> some cleanup that can be done here, or is there a technical reason
> Doug> to handle " different from '?).
>
> Based on my last foray into decode_line_1, I would say that it is likely
> that nobody knows the answer to this question.
>
> That code is seriously twisted and horrible. ?E.g., see PR 8883, or
> 11289, or 11614. ?I think we should plan to rewrite it -- Keith was
> talking about this a while ago... maybe he got scared off ;-)
>
> In the short run I am inclined to approve anything that fixes problems
> and doesn't regress.

IWBN to keep the code at least as coherent as it is now.
Having decode_compound strip quotes doesn't do this for me.
[This feels easy to fix, so I don't see this as asking too much, fwiw,]

> Doug> There are several callers and rather than changing all of them to
> Doug> strip method overloading of the name to search for, it seems
> Doug> reasonable to handle it in lookup_partial_symbol.
>
> One thing I would like to know is the exact semantics required of these
> lookup functions. ?That is, if the implementation of one or more quick
> functions is expected to strip the "(" stuff, then that ought to be
> documented in symfile.h.

I think that, for this particular case, it comes down to having a
definition of what *is* recorded in a psymtab for overloaded methods
(and documenting it of course).

> Doug> IWBN if psymtabs didn't require that complexity and *only* recorded
> Doug> the un-overloaded name.
>
> And then...? ?I suppose we could expand all psymtabs with a matching
> name, then let the symtab code be intelligent about picking out matches.

My premise for saying that is that psymtabs, at least for the case at
hand, *do* store the un-overloaded name.
My point was IWBN to not have them sometimes store un-overloaded names
and sometimes store overloaded names (i.e., don't be inconsistent).
Of course, if they always *only* stored overloaded names then that's
another way, but they don't do that today (and I *think* that's
intentional).

> It is pretty easy to add a pre-expansion call like this. ?This is what
> the index branch does -- it records names only, not symbol types, so
> before a name lookup is done it reads all the CUs that define the name,
> regardless of how it is defined there.
>
> FWIW, Sami is working on an approach like this for template functions.
> I think it is a promising approach, though I'm concerned about how we
> will handle different instantiations of the same unadorned name
> appearing in multiple objfiles.
>
> Doug> AIUI, what you're doing here is having the lookup "fail" if an
> Doug> overloaded-name is found, so that a subsequent search in the full
> Doug> symtab will be done and, having expanded the psymtab here, that search
> Doug> will succeed. ?However psymtabs are searched *after* symtabs. ?This
> Doug> patch works because it turns out that we will later do a search in the
> Doug> full symtab, but that's more by accident than design:
>
> Do you know what exact code path is taken here?

Essentially we're in linespec.c:find_methods, given "foo()" (from
"break c::foo()") and are looking up all the overloaded methods.

Digging deeper, the caller of lookup_partial_symbol for the case at
hand is lookup_symbol_aux_psymtabs, and it does call PSYMTAB_TO_SYMTAB
(as do other callers, except one, but it doesn't need to).  So we
don't have to call PSYMTAB_TO_SYMTAB in lookup_partial_symbol and only
need to return the match.
And it's caller is lookup_symbol_aux_quick ... but we've returned NULL
so it early exits.
Changing lookup_partial_symbol to simply strip the overloading info
before doing the comparison sounds good.

diff -u -p -r1.5 psymtab.c
--- psymtab.c   24 Jun 2010 20:17:52 -0000      1.5
+++ psymtab.c   24 Jun 2010 20:45:03 -0000
@@ -432,6 +432,7 @@ lookup_partial_symbol (struct partial_sy
   struct partial_symbol **top, **real_top, **bottom, **center;
   int length = (global ? pst->n_global_syms : pst->n_static_syms);
   int do_linear_search = 1;
+  char *simple_name = NULL, *paren;

   if (length == 0)
     {
@@ -441,6 +442,16 @@ lookup_partial_symbol (struct partial_sy
           pst->objfile->global_psymbols.list + pst->globals_offset :
           pst->objfile->static_psymbols.list + pst->statics_offset);

+  /* FIXME: What about "(anonymous namespace)".  */
+  paren = strchr (name, '(');
+  if (paren)
+    {
+      simple_name = alloca (strlen (name));
+      memcpy (simple_name, name, paren - name);
+      simple_name[name - paren] = '\0';
+      name = simple_name;
+    }
+
   if (global)                  /* This means we can use a binary search. */
     {
       do_linear_search = 0;

I added the FIXME just as a reminder, I wasn't suggesting checking
this in of course.

> I think what is intended to happen is that a call to a quick function
> may result in symtab expansion. ?Then the caller is supposed to look in
> this symtab for the answer. ?E.g., lookup_symbol_aux_quick does this.

I see that now.

> Doug> This situation is not well solved by our normal psymtabs->symtabs lazy
> Doug> expansion design.
> Doug> I don't have a specific proposal for a better fix at the moment.
> Doug> Comments?
>
> We should probably also plan on a symtab overhaul.

No disagreement here.

> Right now gdb's behavior is dependent on the order of psymtab expansion.
> This can result in weird results for users, even for pretty ordinary
> code. ?I don't think this is very good. ?Pre-expansion would fix this,
> though maybe at a cost; and maybe that alone would introduce new
> problems as well.
>
> Also I think we have some bugs that we could fix if symtab lookups
> returned all the symbols for a given name -- not just the first one
> matching. ?Perhaps we would even need to extend this to work across
> objfiles; I imagine we have plenty of lurking bugs in this scenario.
>
> We may also want to consider the multi-inferior objfile reuse problem
> when doing this. ?Perhaps a new symtab API could return relocated
> symbols or something along those lines.
>
> I'm very interested to learn about other cases where we have trouble.
>
> Tom
>