From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79813 invoked by alias); 22 Aug 2015 17:30:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 79754 invoked by uid 89); 22 Aug 2015 17:30:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=AWL,BAYES_50,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pa0-f53.google.com Received: from mail-pa0-f53.google.com (HELO mail-pa0-f53.google.com) (209.85.220.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sat, 22 Aug 2015 17:29:59 +0000 Received: by padfo6 with SMTP id fo6so29022961pad.1 for ; Sat, 22 Aug 2015 10:29:57 -0700 (PDT) X-Received: by 10.68.112.227 with SMTP id it3mr29931477pbb.132.1440264597473; Sat, 22 Aug 2015 10:29:57 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by smtp.gmail.com with ESMTPSA id b11sm11710843pbu.80.2015.08.22.10.29.56 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 22 Aug 2015 10:29:57 -0700 (PDT) From: Doug Evans To: Pierre-Marie de Rodat Cc: Kevin Buettner , gdb-patches@sourceware.org Subject: Re: [PATCH] Add proper handling for non-local references in nested functions References: <54F47563.4050103@adacore.com> <54FF0D05.70907@redhat.com> <550C1170.9070208@adacore.com> <55685B60.3000004@redhat.com> <55775EB0.4080701@adacore.com> <55AF5F7E.5000600@adacore.com> <20150722173957.7ed51f18@pinnacle.lan> <55B0C583.6050601@adacore.com> <55BB538B.7090104@adacore.com> <55D1E2B5.4000200@adacore.com> Date: Sat, 22 Aug 2015 17:30:00 -0000 In-Reply-To: <55D1E2B5.4000200@adacore.com> (Pierre-Marie de Rodat's message of "Mon, 17 Aug 2015 15:33:41 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00636.txt.bz2 Pierre-Marie de Rodat writes: > Doug, > > Once again, thanks for the review! > > On 08/15/2015 07:08 AM, Doug Evans wrote: >>> +/* Implement the struct symbol_block_ops::get_frame_base method. */ >>> + >>> +static CORE_ADDR >>> +block_op_get_frame_base (struct symbol *framefunc, struct frame_info *frame) >>> +{ >>> + if (SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location != NULL) >> >> ==== >> It's hard for this reader to figure out why this test is here. >> If the code gets here, when would find_frame_base_location be NULL? >> Seems like we could just assert find_frame_base_location is non-NULL. > > Good catch! This is fixed, now, with a comment explaining this. > >>> + /* Old compilers may not provide a static link, or they may provide an >>> + invalid one. For such cases, fallback on the old way to evaluate >>> + non-local references: just climb up the call stack and pick the first >>> + frame that contains the variable we are looking for. */ >>> + if (frame == NULL) >>> + { >>> + frame = block_innermost_frame (var_block); >>> + if (!frame) >> >> ==== >> frame == NULL > > Done. > >> Ok with them fixed (not sure what the right fix for the first one is >> though, I could be missing something). > > As said in my previous mail, I'm not sure I want to commit it: I tried > to investigate this 4% unexpected performance drop, at least to see > what happens with a profiler but I could not reproduce. I'd say let's go ahead. It's certainly possible I'm seeing the difference due to local artifacts. I've tried to rule them out, but TBH I haven't put a whole lot of time into it (beyond trying twice in two different trees). If someone else isn't seeing them then that's good data. I'll play with this a bit more, if it's a local issue it'd be good to find out what it is. :-) > Pierre-Marie de Rodat > > From f8cb12e93bc4b317bf03dd31fc158cc05fc60367 Mon Sep 17 00:00:00 2001 > From: Pierre-Marie de Rodat > Date: Thu, 5 Feb 2015 17:00:06 +0100 > Subject: [PATCH] DWARF: handle non-local references in nested functions > > GDB's current behavior when dealing with non-local references in the > context of nested fuctions is approximative: > > - code using valops.c:value_of_variable read the first available stack > frame that holds the corresponding variable (whereas there can be > multiple candidates for this); > > - code directly relying on read_var_value will instead read non-local > variables in frames where they are not even defined. > > This change adds the necessary context to symbol reads (to get the block > they belong to) and to blocks (the static link property, if any) so that > GDB can make the proper decisions when dealing with non-local varibale > references. > > gdb/ChangeLog: > > * ada-lang.c (ada_read_var_value): Add a var_block argument > and pass it to default_read_var_value. > * block.c (block_static_link): New accessor. > * block.h (block_static_link): Declare it. > * buildsym.c (finish_block_internal): Add a static_link > argument. If there is a static link, associate it to the new > block. > (finish_block): Add a static link argument and pass it to > finish_block_internal. > (end_symtab_get_static_block): Update calls to finish_block and > to finish_block_internal. > (end_symtab_with_blockvector): Update call to > finish_block_internal. > * buildsym.h: Forward-declare struct dynamic_prop. > (struct context_stack): Add a static_link field. > (finish_block): Add a static link argument. > * c-exp.y: Remove an obsolete comment (evaluation of variables > already start from the selected frame, and now they climb *up* > the call stack) and propagate the block information to the > produced expression. > * d-exp.y: Likewise. > * f-exp.y: Likewise. > * go-exp.y: Likewise. > * jv-exp.y: Likewise. > * m2-exp.y: Likewise. > * p-exp.y: Likewise. > * coffread.c (coff_symtab_read): Update calls to finish_block. > * dbxread.c (process_one_symbol): Likewise. > * xcoffread.c (read_xcoff_symtab): Likewise. > * compile/compile-c-symbols.c (convert_one_symbol): Promote the > "sym" parameter to struct block_symbol, update its uses and pass > its block to calls to read_var_value. > (convert_symbol_sym): Update the calls to convert_one_symbol. > * compile/compile-loc2c.c (do_compile_dwarf_expr_to_c): Update > call to read_var_value. > * dwarf2loc.c (block_op_get_frame_base): New. > (dwarf2_block_frame_base_locexpr_funcs): Implement the > get_frame_base method. > (dwarf2_block_frame_base_loclist_funcs): Likewise. > (dwarf2locexpr_baton_eval): Add a frame argument and use it > instead of the selected frame in order to evaluate the > expression. > (dwarf2_evaluate_property): Add a frame argument. Update call > to dwarf2_locexpr_baton_eval to provide a frame in available and > to handle the absence of address stack. > * dwarf2loc.h (dwarf2_evaluate_property): Add a frame argument. > * dwarf2read.c (attr_to_dynamic_prop): Add a forward > declaration. > (read_func_scope): Record any available static link description. > Update call to finish_block. > (read_lexical_block_scope): Update call to finish_block. > * findvar.c (follow_static_link): New. > (get_hosting_frame): New. > (default_read_var_value): Add a var_block argument. Use > get_hosting_frame to handle non-local references. > (read_var_value): Add a var_block argument and pass it to the > LA_READ_VAR_VALUE method. > * gdbtypes.c (resolve_dynamic_range): Update calls to > dwarf2_evaluate_property. > (resolve_dynamic_type_internal): Likewise. > * guile/scm-frame.c (gdbscm_frame_read_var): Update call to > read_var_value, passing it the block coming from symbol lookup. > * guile/scm-symbol.c (gdbscm_symbol_value): Update call to > read_var_value (TODO). > * infcmd.c (finish_command_continuation): Update call to > read_var_value, passing it the block coming from symbol lookup. > * infrun.c (insert_exception_resume_breakpoint): Likewise. > * language.h (struct language_defn): Add a var_block argument to > the LA_READ_VAR_VALUE method. > * objfiles.c (struct static_link_htab_entry): New. > (static_link_htab_entry_hash): New. > (static_link_htab_entry_eq): New. > (objfile_register_static_link): New. > (objfile_lookup_static_link): New. > (free_objfile): Free the STATIC_LINKS hashed map if needed. > * objfiles.h: Include hashtab.h. > (struct objfile): Add a static_links field. > (objfile_register_static_link): New. > (objfile_lookup_static_link): New. > * printcmd.c (print_variable_and_value): Update call to > read_var_value. > * python/py-finishbreakpoint.c (bpfinishpy_init): Likewise. > * python/py-frame.c (frapy_read_var): Update call to > read_var_value, passing it the block coming from symbol lookup. > * python/py-framefilter.c (extract_sym): Add a sym_block > parameter and set the pointed value to NULL (TODO). > (enumerate_args): Update call to extract_sym. > (enumerate_locals): Update calls to extract_sym and to > read_var_value. > * python/py-symbol.c (sympy_value): Update call to > read_var_value (TODO). > * stack.c (read_frame_local): Update call to read_var_value. > (read_frame_arg): Likewise. > (return_command): Likewise. > * symtab.h (struct symbol_block_ops): Add a get_frame_base > method. > (struct symbol): Add a block field. > (SYMBOL_BLOCK): New accessor. > * valops.c (value_of_variable): Remove frame/block handling and > pass the block argument to read_var_value, which does this job > now. > (value_struct_elt_for_reference): Update calls to > read_var_value. > (value_of_this): Pass the block found to read_var_value. > * value.h (read_var_value): Add a var_block argument. > (default_read_var_value): Likewise. > > gdb/testsuite/ChangeLog: > > * gdb.base/nested-subp1.exp: New file. > * gdb.base/nested-subp1.c: New file. > * gdb.base/nested-subp2.exp: New file. > * gdb.base/nested-subp2.c: New file. > * gdb.base/nested-subp3.exp: New file. > * gdb.base/nested-subp3.c: New file. LGTM. Thanks for the patch!