Re: [PATCH] D: Support looking up symbols in the current and imported modules

[Iain Buclaw <ibuclaw at gdcproject dot org>]

On 18 July 2015 at 20:31, Doug Evans <xdje42@gmail.com> wrote:
> Iain Buclaw <ibuclaw@gdcproject.org> writes:
>> Hi,
>>
>> D has the notion of importing modules, whether it is public, private,
>> or static; basic, selective, or renamed.
>>
>> This adds support for looking up symbols in both the current and
>> imported modules so that it is not a necessity to use the qualified
>> name every time.
>>
>> Example:
>>
>> module A;
>>
>> import B;  // Basic import
>> import R = C;  // Renamed import
>> import D : funD;  // Selective import
>> import E : funR = funE;  // Renamed selective import
>>
>> void funA()
>> {
>> // <- Breakpoint here
>> }
>>
>> From the given breakpoint, the following should work in:
>> - All symbols in module 'A' (our current module) can be looked up by name.
>> - All symbols in module 'B' can be looked up by name.
>> - All symbols in module 'C' can be looked up through it's alias R.name.
>> - Only 'funD' in module 'D' can be looked up by name.
>> - Only 'funE' in module 'E' can be looked up through it's alias 'funR'.
>> - All fully qualified symbol names can be looked up.
>>
>>
>> The implementation of this itself is mostly borrowed from
>> cp-namespace.c, but differs in the follow ways:
>> - The separator for modules is a single dot '.'
>> - Renamed selective imports need special handling for D.
>>
>>
>> This has a dependency on dwarf2read.c being able to handle language_d
>> when reading/parsing module/imported declaration symbols, which has
>> been raised as a separate patch.
>>
>> Regards
>> Iain
>>
>> ---
>> 2015-07-14  Iain Buclaw  <ibuclaw@gdcproject.org>
>>
>>       * Makefile.in (SFILES): Add d-namespace.c.
>>       (COMMON_OBS): Add d-namespace.o.
>>       * d-lang.c (d_language_defn): Use d_lookup_symbol_nonlocal as the
>>       la_lookup_symbol_nonlocal callback function pointer.
>>       * d-lang.h (d_lookup_symbol_nonlocal): New declaration.
>>       (d_lookup_nested_symbol): New declaration.
>>       * d-namespace.c: New file.
>
> Hi.
> I didn't study this too much for d-language correctness.
> [If there are issues I'm happy to let them get addressed
> in subsequent patches.]
> LGTM with the nits below fixed.
>
>>
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -835,7 +835,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
>>       charset.c common/cleanups.c cli-out.c coffread.c coff-pe-read.c \
>>       complaints.c completer.c continuations.c corefile.c corelow.c \
>>       cp-abi.c cp-support.c cp-namespace.c cp-valprint.c \
>> -     d-exp.y d-lang.c d-valprint.c \
>> +     d-exp.y d-lang.c d-namespace.c d-valprint.c \
>>       cp-name-parser.y \
>>       dbxread.c demangle.c dictionary.c disasm.c doublest.c \
>>       dtrace-probe.c dummy-frame.c \
>> @@ -1070,7 +1070,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>>       frame-base.o \
>>       inline-frame.o \
>>       gnu-v2-abi.o gnu-v3-abi.o cp-abi.o cp-support.o \
>> -     cp-namespace.o \
>> +     cp-namespace.o d-namespace.o \
>>       reggroups.o \
>>       trad-frame.o \
>>       tramp-frame.o \
>> --- a/gdb/d-lang.c
>> +++ b/gdb/d-lang.c
>> @@ -214,7 +214,7 @@ static const struct language_defn d_language_defn =
>>    default_read_var_value,    /* la_read_var_value */
>>    NULL,                              /* Language specific skip_trampoline.  */
>>    "this",
>> -  basic_lookup_symbol_nonlocal,
>> +  d_lookup_symbol_nonlocal,
>>    basic_lookup_transparent_type,
>>    d_demangle,                        /* Language specific symbol demangler.  */
>>    NULL,                              /* Language specific
>> --- a/gdb/d-lang.h
>> +++ b/gdb/d-lang.h
>> @@ -68,6 +68,16 @@ extern char *d_demangle (const char *mangled, int options);
>>
>>  extern const struct builtin_d_type *builtin_d_type (struct gdbarch *);
>>
>> +/* Defined in d-namespace.c  */
>> +
>> +extern struct symbol *d_lookup_symbol_nonlocal (const struct language_defn *,
>> +                                             const char *,
>> +                                             const struct block *,
>> +                                             const domain_enum);
>> +
>> +extern struct symbol *d_lookup_nested_symbol (struct type *, const char *,
>> +                                           const struct block *);
>> +
>>  /* Defined in d-valprint.c  */
>>
>>  extern void d_val_print (struct type *type, const gdb_byte *valaddr,
>> --- /dev/null
>> +++ b/gdb/d-namespace.c
>> @@ -0,0 +1,574 @@
>> +/* Helper routines for D support in GDB.
>> +
>> +   Copyright (C) 2014-2015 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#include "defs.h"
>> +#include "symtab.h"
>> +#include "block.h"
>> +#include "language.h"
>> +#include "d-lang.h"
>> +#include "cp-support.h"
>
> Is including cp-support.h necessary?
>

Yes, because 'struct using_direct' is declared here, which is used as
part of 'd_lookup_symbol_imports', which is the glue of this patch
that actually does the module import lookups.


>> +/* Look up a symbol named NESTED_NAME that is nested inside the D
>> +   class or module given by PARENT_TYPE, from within the context
>> +   given by BLOCK.  Return NULL if there is no such nested type.  */
>> +
>> +struct symbol *
>
> Make static.
> [Or is there another patch that will use this function?
> E.g., c-exp.y also uses cp_lookup_nested_symbol.]
>

There is another patch that will make use of it that will be sent through.

I've got plans to redo d-exp.y to be more like c-exp.y and have a
two-level lexer to classify modules/types/symbols earlier (alternating
'.' and identifier tokens).  This gives me the chance to open up the
grammar to allow adding of property support, such as exp.sizeof,
exp.typeof, etc.  Something that is not possible in it's current state
without running into shift/reduce conflicts.


>> +    default:
>> +      internal_error (__FILE__, __LINE__,
>> +                   _("d_lookup_nested_symbol called "
>> +                     "on a non-aggregate type."));
>
> gdb_assert_not_reached would be simpler.
>

OK, I'll fix that up.

Regards
Iain.