From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42766 invoked by alias); 24 May 2018 13:33:42 -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 42746 invoked by uid 89); 24 May 2018 13:33:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=99.9, practical X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 24 May 2018 13:33:40 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w4ODXXNg005436 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 24 May 2018 09:33:38 -0400 Received: by simark.ca (Postfix, from userid 112) id E6BC01F21D; Thu, 24 May 2018 09:33:33 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id F29C21E75F; Thu, 24 May 2018 09:33:30 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 24 May 2018 14:35:00 -0000 From: Simon Marchi To: Petr Tesarik Cc: gdb-patches@sourceware.org, Jeff Mahoney Subject: Re: [PATCH] Add an optional offset option to the "symbol-file" command In-Reply-To: <20180523104956.35a01bcc@ezekiel.suse.cz> References: <20180427112449.4e3e3f06@ezekiel.suse.cz> <20180523104956.35a01bcc@ezekiel.suse.cz> Message-ID: <092eb92c83693a6a444ff517c9b0168f@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 24 May 2018 13:33:34 +0000 X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00646.txt.bz2 On 2018-05-23 04:49, Petr Tesarik wrote: > Hi all, > > any comment on my patch? If it's not good, can you elaborate on what > needs improvement, please? > > Petr T Hi Petr, Sorry about that, with the volume on the list patches fall through the cracks some times, it is perfectly appropriate to ping them after a while as you did. I am not against adding that new feature to "symbol-file" (it seems useful), but I am just wondering first if you can achieve the same thing using "add-symbol-file" instead. add-symbol-file doesn't take an offset, but the beginning address of the .text section. Other sections (e.g. .data and .bss) need to be specified separately though. I'd then like to know if it would be possible to make symbol-file similar to add-symbol-file, and if that would be practical/easy to use. On one side, it would be weird if symbol-file and add-symbol-file had different syntaxes to achieve the same thing, but on the other hand having to specify a single offset for all sections of the object file (probably enough 99.9% of the time) sounds much easier than having to specify the base addresses of multiple sections... I don't have big comments on the patch itself, just nits here and there. >> @@ -1222,16 +1222,20 @@ symbol_file_add (const char *name, >> symfile_add_flags add_flags, >> void >> symbol_file_add_main (const char *args, symfile_add_flags add_flags) >> { >> - symbol_file_add_main_1 (args, add_flags, 0); >> + symbol_file_add_main_1 (args, add_flags, 0, 0); >> } >> >> static void >> symbol_file_add_main_1 (const char *args, symfile_add_flags >> add_flags, >> - objfile_flags flags) >> + objfile_flags flags, CORE_ADDR offset) >> { >> + struct objfile *objfile; >> + >> add_flags |= current_inferior ()->symfile_flags | SYMFILE_MAINLINE; >> >> - symbol_file_add (args, add_flags, NULL, flags); >> + objfile = symbol_file_add (args, add_flags, NULL, flags); You can declare and assign the variable on the same line. >> @@ -1568,6 +1579,8 @@ symbol_file_command (const char *args, int >> from_tty) >> flags |= OBJF_READNOW; >> else if (strcmp (arg, "-readnever") == 0) >> flags |= OBJF_READNEVER; >> + else if (strcmp (arg, "-o") == 0) >> + expecting_offset = true; This doesn't handle correctly (IMO) "symbol-file foo -o", which should be rejected with an error message. I think it would be simpler to do something like this: else if (strcmp (arg, "-o") == 0) { arg = built_argv[++idx]; if (arg == NULL) error (_("Missing argument to -o")); offset = parse_and_eval_address (arg); } >> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog >> index 34da102c62..68431cb035 100644 >> --- a/gdb/testsuite/ChangeLog >> +++ b/gdb/testsuite/ChangeLog >> @@ -1,3 +1,7 @@ >> +2018-04-27 Petr Tesarik >> + >> + * gdb.base/relocate.exp: Add test for "symbol-file -o ". >> + >> 2018-04-26 Pedro Alves >> >> * gdb.base/gnu-ifunc.exp (set-break): Test that GDB resolves >> diff --git a/gdb/testsuite/gdb.base/relocate.exp >> b/gdb/testsuite/gdb.base/relocate.exp >> index 89f2fffcd9..4383e79cb2 100644 >> --- a/gdb/testsuite/gdb.base/relocate.exp >> +++ b/gdb/testsuite/gdb.base/relocate.exp >> @@ -196,6 +196,39 @@ if { "${function_foo_addr}" == >> "${new_function_foo_addr}" } { >> pass "function foo has a different address" >> } >> >> +# Load the object using symbol-file with an offset and check that >> +# all addresses are moved by that offset. >> + >> +set offset 0x10000 >> +clean_restart >> +gdb_test "symbol-file -o $offset $binfile" \ >> + "Reading symbols from ${binfile}\.\.\.done\." \ >> + "symbol-file with offset" >> + >> +# Make sure the address of a static variable is moved by offset. >> +set new_static_foo_addr [get_var_address static_foo] >> +if { "${new_static_foo_addr}" == "${static_foo_addr}" + $offset } { >> + pass "static variable foo is moved by offset" >> +} else { >> + fail "static variable foo is moved by offset" >> +} You can simplify these using gdb_assert: gdb_assert "${new_static_foo_addr} == ${static_foo_addr} + $offset" \ "static variable foo is moved by offset" Thanks, Simon