From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104617 invoked by alias); 30 Oct 2017 23:38:29 -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 104604 invoked by uid 89); 30 Oct 2017 23:38:29 -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,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Mon, 30 Oct 2017 23:38:27 +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 v9UNcKjK018457 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 30 Oct 2017 19:38:25 -0400 Received: by simark.ca (Postfix, from userid 112) id 08E981E526; Mon, 30 Oct 2017 19:38:19 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 82B6C1E4F3; Mon, 30 Oct 2017 19:37:58 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 30 Oct 2017 23:38:00 -0000 From: Simon Marchi To: Mike Gulick Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [RFC][PATCH] fix gdb segv when objfile can't be opened In-Reply-To: <82a1c23a-37f4-dee5-beae-da95d1cc1c6c@mathworks.com> References: <59E8B251.4050100@mathworks.com> <8c08307a-94ad-92b8-9c8b-c713cad541fd@mathworks.com> <56b1cb34b33613ca4496abfcd28f135a@polymtl.ca> <46f29b7d-852e-8db1-d7e7-d59c5857d3c0@ericsson.com> <82a1c23a-37f4-dee5-beae-da95d1cc1c6c@mathworks.com> Message-ID: <9ee6ad949b591f29f5205d00ce8527ff@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 30 Oct 2017 23:38:20 +0000 X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00910.txt.bz2 On 2017-10-30 18:13, Mike Gulick wrote: > On 10/27/2017 09:17 PM, Simon Marchi wrote: >> On 2017-10-23 07:19 PM, Mike Gulick wrote: >> >> ... >> >> Thanks a lot. Could you also write the corresponding ChangeLog >> entries? See >> here for details: http://sourceware.org/gdb/wiki/ContributionChecklist >> Note that we usually put that as part of the commit log, and only move >> it >> to the actual ChangeLog files before pushing to git. The changes to >> the >> source files will go in gdb/ChangeLog and the changes to the testsuite >> in >> gdb/testsuite/ChangeLog. >> >> You can also put the PR number as part of the ChangeLog (see wiki and >> previous CL entries for examples). >> > Done. > >> I don't think you have a copyright assignment for GDB yet? Since this >> is >> more than a few lines, you will need one if you want to contribute >> (I'll >> contact you off-list for that). >> > I'm working on this. Hopefully it will be sooner rather than later. > >> I noted a a few comments below (mostly minor/formatting stuff, overall >> I >> think this is a good quality submission). >> ... >>> >>> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c >>> index cc02740..b33de07 100644 >>> --- a/gdb/gdb_bfd.c >>> +++ b/gdb/gdb_bfd.c >>> @@ -702,9 +702,14 @@ gdb_bfd_map_section (asection *sectp, >>> bfd_size_type *size) >>> >>> data = NULL; >>> if (!bfd_get_full_section_contents (abfd, sectp, &data)) >>> - error (_("Can't read data for section '%s' in file '%s'"), >>> - bfd_get_section_name (abfd, sectp), >>> - bfd_get_filename (abfd)); >>> + { >>> + warning (_("Can't read data for section '%s' in file '%s'"), >>> + bfd_get_section_name (abfd, sectp), >>> + bfd_get_filename (abfd)); >>> + /* Section is invalid -- set size to 0 and return NULL */ >> >> Finish the comment with a dot and two spaces. >> > I rewrote the comment to be a full sentence Ok. >>> ... >>> +extern int bar (int); >>> + >>> +int foo (int x) { >>> + return bar (x); >>> +} >> >> Unless the purpose of the test is about formatting, we try to follow >> GNU >> style (same as for source code) in tests. For example, here it would >> be: >> >> int >> foo (int x) >> { >> return bar (x); >> } >> > I reformatted all of the helper files for the test using 'indent'. Great, thanks. >>> ... >>> +int bar (int y) { >>> + return y + 1; /* break here */ >>> +} >> >> Same here. >> >>> ... >>> +#ifndef VANISH_LIB >>> +#define VANISH_LIB "./solib-vanish-lib1.so" >>> +#endif >> >> I think we can remove this, it could only obfuscate a problem >> if we remove the -DVANISH_LIB in the .exp file. >> > Done. > >>> + >>> +int main() >> >> Put "int" on its own line. >> >>> +{ >>> + void *handle; >>> + int (*foo)(int); >>> + char *error; >>> + >>> + /* Open library */ >>> + handle = dlopen(VANISH_LIB, RTLD_NOW); >>> + if (!handle) { >>> + fprintf(stderr, "%s\n", dlerror()); >> >> Please make this file follow GNU style as well (space before >> parenthesis, >> curly braces position, etc). >> >>> + exit(EXIT_FAILURE); >>> + } >>> + >>> + /* Clear any existing error */ >>> + dlerror(); >>> + >>> + /* Remove library */ >>> + if (remove(VANISH_LIB) == -1) { >> >> Can we rename the lib instead of deleting it? It's always preferable >> to keep >> test artifacts in case we need to inspect them. >> > I updated the test to rename the library instead of removing it. It > also now renames it > back before exiting. This *should* make the test idempotent, unless > it fails before it > is able to rename the .so back to its original name. The test (the .exp) rebuilds the shared library every time anyway, so even if you didn't move it back, it would work fine. >>> ... >>> + >>> +# Library 1 >>> +set lib1name "solib-vanish-lib1" >>> +set srcfile_lib1 ${srcdir}/${subdir}/${lib1name}.c >>> +set binfile_lib1 [standard_output_file ${lib1name}.so] >>> +# I can't make this work for some reason: >>> +#set lib1_flags [list debug shlib=${binfile_lib2}] >>> +set lib1_flags [list debug ldflags=-l:${lib2name}.so >>> ldflags=-Wl,-rpath=[standard_output_file ""] >>> libdir=[standard_output_file ""]] >> >> With >> >> https://sourceware.org/ml/gdb-patches/2017-10/msg00856.html >> >> the commented out line should work. >> > This did indeed work. Thank you! > >>> ... >>> + >>> +gdb_test "continue" \ >>> + "Breakpoint .*at .*/${lib2name}.c:${lib2_lineno}.*break here.*" >>> \ >>> + "breakpoint prints location" >> >> I think you could use the "gdb_continue_to_breakpoint" proc. >> > This works as well, so the new patch uses this instead. > > Here's an updated patch which hopefully addresses all of these items. > > ... Thanks for the update, this LGTM. Please ping when your hear back about your copyright assignment. Simon