From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119604 invoked by alias); 14 Jun 2017 16:27:00 -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 119542 invoked by uid 89); 14 Jun 2017 16:26:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=axis, noticeable, family, our X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Jun 2017 16:26:57 +0000 Received: by simark.ca (Postfix, from userid 33) id C4C1B1E4B5; Wed, 14 Jun 2017 12:27:00 -0400 (EDT) To: Anton Kolesov Subject: Re: [PATCH v2] arc: Select CPU model properly before disassembling X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 14 Jun 2017 16:27:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org, Francois Bedard In-Reply-To: <39A54937CC95F24AA2F794E2D2B66B1358756E48@DE02WEMBXB.internal.synopsys.com> References: <39A54937CC95F24AA2F794E2D2B66B135874F238@DE02WEMBXB.internal.synopsys.com> <20170613134951.29359-1-Anton.Kolesov@synopsys.com> <887411aaab641fb92f809eadce4d9011@polymtl.ca> <39A54937CC95F24AA2F794E2D2B66B1358756E48@DE02WEMBXB.internal.synopsys.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00428.txt.bz2 On 2017-06-14 16:02, Anton Kolesov wrote: > This is the same thing as in {arm,rs6000,s390-linux}-tdep.c - they all > use > global static variable, which would be shared across multiple gdbarch > instances. So when options are changed that would change that for any > gdbarch > of that architecture. RS6000 and S390 don't even assign this variable > any value > - it completely managed by the disasm.c. Although in ARC case there is > a memory > leak, because arc_disassembler_options is only assigned by arc-tdep.c > and > never freed - that's because I didn't properly understood what other > arches > do - they don't free options as well, but they also don't allocate them > in > each gdbarch_init, because there usecase is somewhat different. Indeed, set_disassembler_options takes care of the allocation/deallocation when the user changes the options. > I'm not sure > how big of a problem this leak is, because outside of a GDB test suite > I don't > think there are a lot of practical cases where same GDB instance is > reused to > connect/disconnect, so leak wouldn't be noticeable. I think, I can make > things better for ARC, by moving arc_disassembler_options into the > gdbarch_tdep, then each gdbarch for ARC will have its own options, but > that > would mean behavior different from other arches which support > disassembler_options - there options are shared across gdbarches of > architecture. Should I do that in V3? Ah, I did not understand previously that sharing the disassembler options between all gdbarches of an architecture is the intended behavior. Having different behavior across architectures for this wouldn't be a good idea, I think. Also, as of today, the disassembler options are very much user driven. The architecture family can set some default at startup (ARM, for example, sets "reg-names-std"), but after that it's all up to the user. In your case, it's the opposite along those two axis: you want GDB to set disassembler options in the back of the user, and you want disassembler options (at least some of them) to be gdbarch-specific. To support that correctly, I think we should first define what behavior we want from GDB. For example: - if the user doesn't specify any disassembler-options, it's probably right to automatically set it for them. - if the user provides a disassembler option other than cpu= and then connects to a gdbserver, we probably want to add the cpu= to what they have specified. - if they connect first and then set another, unrelated option, do we keep the cpu= option or does it get lost? - if they set a particular cpu= option and then connect to a gdbserver that reports something else, which one do we choose? - if they connect to a gdbserver that reports an architecture and we add a cpu= for it, then they connect to a gdbserver that doesn't report an architecture, do we keep the previous cpu= or not? - what happens with the various options when we switch gdbarch, do we keep the user-provided ones but flush the GDB-inferred ones? Once we know what behavior we want from GDB, I think it will be easier to decide what to put where. In the mean time, if you think the current patch makes the situation better for ARC users in the typical usage scenarios, I'm fine with going with it. Note that there will be a change for your users: connecting to a target will overwrite whatever option they might have set before connecting. Also, to avoid the leak, you could xfree the previous arc_disassembler_options before assigning it in arc_gdbarch_init. > TBH, that would be a moot for ARC right now, because today our > disassembler > has a global state and it doesn't support simultaneous disassembly of > different ARC architectures anyway, but it might make sense to try to > make > sure that GDB part handles this correctly, if in the future > disassembler > would be upgraded to avoid global state. > > The reuse of gdbarch instances seems like a good idea, will try to add > that > in the future for ARC. > > Anton Thanks, Simon