On 12/13/2017 10:36 AM, Joel Brobecker wrote:
> Hi Pedro,
> 
> I don't know if you remember about the patch series that introduces
> wild matching for C++ as well? I havent' verified this in details,
> because there is a series of patches, and they are fairly long, but
> I have a feeling that they may have introduced a regression. I can
> bisect tomorrow if needed.
> 
> In any case, consider the following code which first declares
> a tagged type (the equivalent of a class in Ada), and then
> a procedure which takes a pointer (access) to this type's 'Class.
> 
>     package Pck is
>        type Top_T is tagged record
>           N : Integer := 1;
>        end record;
>        procedure Inspect (Obj: access Top_T'Class);
>     end Pck;
> 
> Putting a breakpoint in that procedure and then running to it triggers
> an internal error:
> 
>     (gdb) break inspect
>     (gdb) continue
>     Breakpoint 1, pck.inspect (obj=0x63e010
>     /[...]/gdb/stack.c:621: internal-error: void print_frame_args(symbol*, frame_info*, int, ui_file*): Assertion `nsym != NULL' failed.
> 
> What's special about this subprogram is that it takes an access
> to what we call a 'Class type, and for implementation reasons,
> the compiler adds an extra argument named "objL". If you are
> curious why, it allows the compiler for perform dynamic accessibility
> checks that are mandated by the language.
> 
> If we look at the location where we get the internal error
> (in stack.c), we find that we are looping over the symbol of
> each parameter, and for each parameter, we do:
> 
>     /* We have to look up the symbol because arguments can have
>        two entries (one a parameter, one a local) and the one we
>        want is the local, which lookup_symbol will find for us.
>     [...]
>         nsym = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
>                               b, VAR_DOMAIN, NULL).symbol;
>         gdb_assert (nsym != NULL);
> 
> The lookup_symbol goes through the lookup structure, which means
> the symbol's linkage name ("objL") gets transformed into
> a lookup_name_info object (in block_lookup_symbol), before
> it gets fed to the block symbol dictionary iterators. This,
> in turn, triggers the symbol matching by comparing the
> "lookup" name which, for Ada, means among other things, lowercasing
> the given name to "objl". It is this transformation that causes
> the lookup find no matches, and therefore trip this assertion.
> 
> Going back to the "offending" call to lookup_symbol in stack.c,
> what we are trying to do, here, is do a lookup by linkage name.
> So, I think what we mean to be doing is a completely litteral
> symbol lookup, so maybe not even strcmp_iw, but actually just
> plain strcmp???
> 
> There doesn't seem to be a way to do just that anymore,
> unfortunately. While this wasn't necessarily part of the spec,
> in the past, in practice, you could get that effect by doing
> a lookup using the C language. But that doesn't work, because
> we still end up somehow using Ada's lookup_name routine which
> transforms "objL".
> 
> So, ideally, as I hinted before, I think what we need is a way
> to perform a litteral lookup so that searches by linkage names
> like the above can be performed. But this might be a fairly
> involved change (maybe adding a new kind of lookup, and then
> making adjustments so we know which kind of name to use for
> name matching).

Indeed.  

I gave the "literal" lookup type a try today, and it turns out
not being so bad, I think.  See patch below.

Thinking through this and experimenting a bit, I think I convinced
myself that with the current symbol tables infrastructure,
it's not literal _linkage_ names that we want to pass down, but
instead literal _search_ names.  I.e., notice that I've switched
the lookup_symbol call in question to pass down
SYMBOL_SEARCH_NAME instead of SYMBOL_LINKAGE_NAME.  See
comments in symtab.h in the patch.

I was thinking about trying to add a symbol_name_match_type::LINKAGE
and support searching by linkage name for any language, but the
thing is that the dictionaries only work with SYMBOL_SEARCH_NAME,
AFAICT, because that's what is used for hashing.  We'd need
separate dictionaries for hashed linkage names.

The patch is not complete in the sense that there are
symbol-lookup entry points that could be tweaked to pass
down a symbol_name_match_type instead of assuming FULL.

And also, I know there are places in ada-lang.c that are doing
what you were doing here too (the "<...>" verbatim trick) which
could be adjusted.  But at least this fixes your testcase too,
and causes no regressions.  So maybe we could do this incrementally
and pass adjust functions to pass around a symbol_name_match_type
as we find a need?  Functions that we miss passing down simply
end up behaving like symbols_name_match_type::FULL, as today.

> Can you tell me what you think?

Your turn.  :-)

> 
> I'm sorry I didn't notice this when I did my review and round
> of testing. I did the best I could, but the current version of
> GDB showing 300+ failures in AdaCore's testsuite at that time,
> I used manual report comparison, and I must have missed
> these differences (7 failures).

Well, no worries for me.  :-)

> Note also that I consider this issue blocking for 8.1, as these
> are not just limited to access to 'Class parameters. These kinds
> of internal parameters can also be generated in other situations,
> and in particular when using tasking (the equivalent of threads).
> Tasking is fairly prevalent in Ada programs. We might even want
> to defer branching if we elect to take the more general fix of
> adding litteral lookup support...

Thanks,
Pedro Alves