From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25517 invoked by alias); 20 Jun 2018 15:40:35 -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 25477 invoked by uid 89); 20 Jun 2018 15:40:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=Wire, HTo:D*ca X-Spam-User: qpsmtpd, 2 recipients X-HELO: 9pmail.ess.barracuda.com Received: from 9pmail.ess.barracuda.com (HELO 9pmail.ess.barracuda.com) (64.235.150.224) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Jun 2018 15:40:32 +0000 Received: from mipsdag02.mipstec.com (mail2.mips.com [12.201.5.32]) by mx28.ess.sfj.cudaops.com (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA256 bits=128 verify=NO); Wed, 20 Jun 2018 15:40:20 +0000 Received: from [10.20.78.223] (10.20.78.223) by mipsdag02.mipstec.com (10.20.40.47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1415.2; Wed, 20 Jun 2018 08:40:16 -0700 Date: Wed, 20 Jun 2018 15:40:00 -0000 From: "Maciej W. Rozycki" To: Simon Marchi CC: , , Joel Brobecker , Fredrik Noring Subject: Re: [PP?] [PATCH v3] GDB PR tdep/8282: MIPS: Wire in `set disassembler-options' In-Reply-To: <158bad5a-3a2d-cc7f-4a77-71135a93f0ac@polymtl.ca> Message-ID: References: <158bad5a-3a2d-cc7f-4a77-71135a93f0ac@polymtl.ca> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-ClientProxiedBy: mipsdag02.mipstec.com (10.20.40.47) To mipsdag02.mipstec.com (10.20.40.47) X-BESS-ID: 1529509219-637138-3833-42531-1 X-BESS-VER: 2018.7-r1806151722 X-BESS-Apparent-Source-IP: 12.201.5.32 X-BESS-Envelope-From: Maciej.Rozycki@mips.com X-BESS-Outbound-Spam-Score: 0.00 X-BESS-Outbound-Spam-Status: SCORE=0.00 using account:ESS59374 scores of KILL_LEVEL=7.0 tests=BSF_BESS_OUTBOUND X-BESS-Orig-Rcpt: simon.marchi@polymtl.ca,gdb-patches@sourceware.org,binutils@sourceware.org,brobecker@adacore.com,noring@nocrew.org X-BESS-BRTS-Status:1 X-SW-Source: 2018-06/txt/msg00496.txt.bz2 Hi Simon, > > Existing affected target backends have been adjusted accordingly. The > > only other backend (besides MIPS, now modernized) setting disassembler > > options in its own specific way turned out to be the i386 one. As its > > `i386_print_insn' handler is always passed a NULL `disassembler_options' > > value, assert that and clear the pointer after use, so that code in > > `gdb_buffered_insn_length' doesn't choke on `free' being called on a > > static data pointer. > > Having a field sometimes own the string and sometimes don't is a recipe for > confusion. Indeed, however I decided not to complicate the thing further under an assumption (perhaps an unjustified one) that the i386 backend will eventually get converted to support `set disassembler-options ...' as well. At which point the hack would go away naturally. > It would be better to make get_all_disassembler_options return a > gdb::unique_xmalloc_ptr instead [1], and propagate to whatever object/scope really > owns the string. See the patch at the bottom implementing this idea (applied on > top of this one). If you however think it's worth an extra effort to support the interim situation with the i386 backend, then I can't object. Thanks for this update. > [1] Or an std::string. but that does not play well with remove_whitespace_and_extra_commas. > Can we avoid calling remove_whitespace_and_extra_commas by not putting the comma if it > is not necessary? Yeah, why not. > > Index: binutils/gdb/disasm.c > > =================================================================== > > --- binutils.orig/gdb/disasm.c 2018-06-13 16:00:05.000000000 +0100 > > +++ binutils/gdb/disasm.c 2018-06-14 21:45:24.771254073 +0100 > > @@ -722,6 +722,35 @@ fprintf_disasm (void *stream, const char > > return 0; > > } > > > > +/* Combine implicit and user disassembler options and return them > > + in a newly-allocated buffer. Return NULL if the string would be > > + empty. */ > > + > > +static const char * > > +get_all_disassembler_options (struct gdbarch *gdbarch) > > +{ > > + const char *implicit_options; > > + const char *options; > > + char *all_options; > > + size_t len; > > + > > + implicit_options = gdbarch_disassembler_options_implicit (gdbarch); > > + options = get_disassembler_options (gdbarch); > > + len = ((implicit_options != NULL ? strlen (implicit_options) : 0) + 1 > > + + (options != NULL ? strlen (options) : 0) + 1); > > + all_options = (char *) xmalloc (len); > > + sprintf (all_options, "%s,%s", > > + implicit_options != NULL ? implicit_options : "", > > + options != NULL ? options : ""); > > It would be better to use xstrprintf instead of computing the required length > by hand. Good point, I didn't know of this one (or for that matter, the underlying `asprintf'/`vasprintf' functions, which are a GNU extension). Thanks. I'll fold your change into my proposal and rewrite the necessary bits to use `std::string'. Thank you for your review. Maciej