From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21888 invoked by alias); 25 May 2018 05:23:31 -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 21869 invoked by uid 89); 25 May 2018 05:23:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=honestly, switched, inconvenient, conference X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 25 May 2018 05:23:29 +0000 Received: from relay1.suse.de (charybdis-ext-too.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 2619FAD4E; Fri, 25 May 2018 05:23:27 +0000 (UTC) Date: Fri, 25 May 2018 11:41:00 -0000 From: Petr Tesarik To: Simon Marchi Cc: gdb-patches@sourceware.org, Jeff Mahoney Subject: Re: [PATCH] Add an optional offset option to the "symbol-file" command Message-ID: <20180525072312.3d50b6b3@ezekiel.suse.cz> In-Reply-To: <092eb92c83693a6a444ff517c9b0168f@polymtl.ca> References: <20180427112449.4e3e3f06@ezekiel.suse.cz> <20180523104956.35a01bcc@ezekiel.suse.cz> <092eb92c83693a6a444ff517c9b0168f@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00659.txt.bz2 Hi Simon, On Thu, 24 May 2018 09:33:30 -0400 Simon Marchi wrote: >[...] > 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. No problem. > 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... You bet! In fact, I was kind of expecting this question. Yes, technically, add-symbol-file can be used for the same purpose, but it is very inconvenient. Most notably, it requires listing all ELF sections explicitly, and the Linux kernel has a lot of them (typically a few dozen). So, my current solution is: 1. exec-file vmlinux 2. info target 3. # parse the output, adding an offset to each section's start 4. add-symbol-file vmlinux $textaddr -s ... # a very long list 5. exec-file # to make sure that only target memory is used Although I have already written a Python script to initialise the session, it's ugly, especially when it comes to parsing the output of "info target". Regarding consistency, add-symbol-file does not currently have any conflicting "-o" option, so I can add one for the same purpose. Shall I do that? > I don't have big comments on the patch itself, just nits here and > there. >[...] > >> - 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. Indeed. I tend to forget that gdb has switched to C++. > >> @@ -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); > } Ah, so that's how it's done. Honestly, I was quite surprised to find no variant of getopt() here... >[...] > >> +# 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" Will do. I should probably simplify other similar stanzas in the test suite in a separate patch. Thank you for your review! I will send an improved patch soon (but maybe not today, as I'm at a conference). Petr T