Re: Fix gdb.ada/bp_c_mixed_case.exp (PR gdb/22670) (Re: [PATCH 3/3] Add new gdb.ada/bp_c_mixed_case testcase for PR gdb/22670)

[Pedro Alves <palves@redhat.com>]

On 01/09/2018 09:46 AM, Joel Brobecker wrote:
>>> I am wondering why minimal symbols are involved in this case,
>>> considering that the C file was build with debugging information.
>>> Shouldn't we be getting the function's address from the partial/full
>>> symtabs instead?
>>
>> AFAIK, GDB always worked this way for linespecs, even before my C++
>> wildmatching patches -- we collect symbols from both debug info and
>> minsyms, and coalesce them by address to avoid duplicates
>> (linespec.c:add_matching_symbols_to_info).  
> 
> That's true.
> 
> What surprises me is that, before your patch, we were finding
> no symbol at all. So we were failing the lookup both with minimal
> symbols, and within the partial/full symtab.

Indeed, good point.  I don't know what I did not think of that.

> 
> Your patch, IIUC, handles the lookup at the minimal symbol level,
> which is indeed a good thing. But shouldn't we also be finding
> that same symbol through the partial/full symtab search? I have
> a feeling that your minimal symbol patch might be hiding a bug
> in the search for the symbol, at least from the linespec module.
> 
> I did a bit of debugging this morning, first with the following
> snapshot, which is shortly before the wild-matching patch series:
> 
>     commit b346cb961f729e2955391513a5b05eaf02b308ea
>     Author: GDB Administrator <gdbadmin@sourceware.org>
>     Date:   Wed Nov 8 00:00:20 2017 +0000
> 
> The function iterate_over_all_matching_symtabs finds the function
> in the bar.c's partial symtab because the matching function is...
> 
>            [&] (const char *symbol_name)
>            {
>              return symbol_name_cmp (symbol_name, name) == 0;
>            },
> 
> ... where name, in this case is "MixedCaseFunc" -- The "<>" has been
> stripped. They got stripped by linespec.c::find_linespec_symbols
> when it took that name and converted it to a lookup name via:
> 
>   if (state->language->la_language == language_ada)
>     {
>       /* In Ada, the symbol lookups are performed using the encoded
>          name rather than the demangled name.  */
>       ada_lookup_storage = ada_name_for_lookup (name);
>       lookup_name = ada_lookup_storage.c_str ();
>     }
>   else
>     {
>       lookup_name = demangle_for_lookup (name,
>                                          state->language->la_language,
>                                          demangle_storage);
>     }
> 
> In the newer version, find_linespec_symbols gets passed the lookup_name
> directly, and that lookup_name is now "<MixedCaseFunc>". Those extra
> "<...>" are what eventually gets in the way when we compare this
> lookup_name against the partial's symbols name (in
> default_symbol_name_matcher, which does an strncmp_iw_with_mode
> comparison, IIUC).
> 
> The call to find_linespec_symbols comes from linespace_parse_basic,
> which has:
> 
>   /* Try looking it up as a function/method.  */
>   find_linespec_symbols (PARSER_STATE (parser),
>                          PARSER_RESULT (parser)->file_symtabs, name,
>                          PARSER_EXPLICIT (parser)->func_name_match_type,
>                          &symbols, &minimal_symbols);
> 
> I really hate to be stopping the investigation at this point, as
> I feel I am onto something, but I am running out of time for today.
> 
> The part where I am not sure yet is whether we should be transforming
> "name" into a "lookup_name" before calling find_linespec_symbols, or
> whether we should be handling the angle brackets during the symbol
> comparison... Or something else entirely! This is still all fairly
> new to me...
> 
> Note that I was thinkg we would need to be stripping the executable
> for us to demonstrate an error, but in fact, this is what happens
> if I use "print" instead of "break":
> 
>     (gdb) p <MixedCaseFunc>
>     $1 = {<text variable, no debug info>} 0x4024dc <MixedCaseFunc>
> 
> With the snapshot prior to the patch series, GDB knows that
> MixedCaseFunc is a function without parameters, and the expression
> above means calling it. As I was debugging without having started
> the inferior, I got the following (expected) error:
> 
>     (gdb) print  <MixedCaseFunc>
>     You can't do that without a process to debug.
> 
> in the bp_c_mixed_case.exp, we should see GDB telling us that
> we stopped on our MixedCaseFunc breakpoint while evaluating
> a function call...
> 
> Does this make some kind of sense to you? 

Yes it does.  I played with this a bit, and am testing a patch.
Stay tuned.

> I can get back to this
> for more digging again tomorrow.
Thanks,
Pedro Alves