From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8D0823857C6A for ; Wed, 22 Jul 2020 14:54:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8D0823857C6A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D42271E111; Wed, 22 Jul 2020 10:54:26 -0400 (EDT) Subject: Re: [PATCH v3 1/3] arc: Add ARCv2 XML target along with refactoring To: Shahab Vahedi Cc: gdb-patches@sourceware.org, Shahab Vahedi , Tom Tromey , Anton Kolesov , Francois Bedard References: <20200428160437.1585-1-shahab.vahedi@gmail.com> <20200713154527.13430-1-shahab.vahedi@gmail.com> <20200713154527.13430-2-shahab.vahedi@gmail.com> <422b0905-456f-8722-b74d-97ade4a95c58@simark.ca> <20200715203500.GA4811@gmail.com> <20200722133641.GB10630@gmail.com> <0731a886-0896-0776-11a9-94f44810eb75@simark.ca> <20200722143345.GC10630@gmail.com> From: Simon Marchi Message-ID: <6b1f28b0-7efa-72ed-b14c-b302405cc4b6@simark.ca> Date: Wed, 22 Jul 2020 10:54:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200722143345.GC10630@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jul 2020 14:54:31 -0000 On 2020-07-22 10:33 a.m., Shahab Vahedi wrote: > On Wed, Jul 22, 2020 at 09:49:48AM -0400, Simon Marchi wrote: >> On 2020-07-22 9:36 a.m., Shahab Vahedi wrote: >>> On Thu, Jul 16, 2020 at 09:28:50AM -0400, Simon Marchi wrote: >>>> On 2020-07-15 4:35 p.m., Shahab Vahedi wrote: >>>> When I search for `arc_gdbarch_features_init` in that patch I don't find anything... >>> >>> You're correct. I have it in my local branch. The patch that uses it is going >>> to be submitted very soon. I hope you don't mind if it remains like this. >> >> Let's just make it static in this patch, and make in non-static again in your future >> patch, it's not a big change. > > Will do. >> >>>> The code in GDB constructing an arc_gdbarch_features will use the BFD to determine >>>> these two values, whereas the code in GDBserver will use something else (usually >>>> looking at the current process' properties). >>>> >>>> In fact, the constructor is optional, you could just build a arc_gdbarch_features >>>> using aggregate initialization and return it from that function: >>>> >>>> arc_gdbarch_features features {reg_size, isa}; >>>> >>>> It doesn't really matter. I just happen to prefer the constructor method, because >>>> that makes it so you can't "forget" a field and it ensures it can never be in an >>>> uninitialized state. >>> >>> I see now. Actually, I have usecases to not initialize it immediately, >>> but in a few lines of code. >> >> I'd be curious to see. Because instead of declaring it immediately, you can keep >> the fields separate until you initialize it: >> >> int reg_size; >> enum arc_isa isa; >> >> if (something) >> { >> reg_size = 8; >> isa = foo; >> } >> else >> { >> reg_size = 4; >> isa = bar; >> } >> >> arc_gdbarch_features features (reg_size, isa); >> >> I think this is good, because the day you add a third axis to arc_gdbarch_features, >> that code will not build and you'll be forced to updated it (you can't forget it). >> >> Whereas with: >> >> arc_gdbarch_features features; >> >> if (something) >> { >> features.reg_size = 8; >> features.isa = foo; >> } >> else >> { >> features.reg_size = 4; >> features.isa = bar; >> } >> >> It's possible to forget. >> >> But maybe you have a different use case in mind? > > These are the 2 usecases I have: > > gdb/arc-tdep.c > -------------- > static bool > arc_tdesc_init (struct gdbarch_info info, ...) > { > const struct target_desc *tdesc_loc = info.target_desc; > > if (!tdesc_has_registers (tdesc_loc)) > { > arc_gdbarch_features features; > arc_gdbarch_features_init (features, info.abfd, > info.bfd_arch_info->mach); > tdesc_loc = arc_lookup_target_description (features); > } > ... > } > > gdb/arc-linux-tdep.c > -------------------- > static const struct target_desc * > arc_linux_core_read_description (struct gdbarch *gdbarch, > struct target_ops *target, > bfd *abfd) > { > arc_gdbarch_features features; > arc_gdbarch_features_init (features, abfd, > gdbarch_bfd_arch_info (gdbarch)->mach); > return arc_lookup_target_description (features); > } I mentioned earlier that arc_gdbarch_features_init should return a `arc_gdbarch_features` by value, that would avoid this problem (and be cleaner, IMO). > While I totally understand your point, I don't want to unroll the > logic of "arc_gdbarch_features_init ()" to the same level that > "features" is declared. Ideally, I should have a constructor > with the same signature as "arc_gdbarch_features_init ()", but > that is not possible because compilation of gdbserver will go > awry when there is mention of ELF data in "gdb/arch/arc.h". > > To have a complete overview, in a soon-to-be-submitted patch > you're going to see that "features" is initialized like: > > gdbserver/linux-arc-low.cc > -------------------------- > static const struct target_desc * > arc_linux_read_description (void) > { > struct target_desc *tdesc; > arc_gdbarch_features features = {4, ARC_ISA_NONE}; > #ifdef __ARC700__ > features.isa = ARC_ISA_ARCV1; > #else > features.isa = ARC_ISA_ARCV2; > #endif > tdesc = arc_create_target_description (features); > ... > } You can easily apply the suggestion from my previous message here. Simon