From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x643.google.com (mail-ej1-x643.google.com [IPv6:2a00:1450:4864:20::643]) by sourceware.org (Postfix) with ESMTPS id D9B9F3857029 for ; Wed, 22 Jul 2020 14:33:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D9B9F3857029 Received: by mail-ej1-x643.google.com with SMTP id rk21so2465032ejb.2 for ; Wed, 22 Jul 2020 07:33:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=3t+woai6AgjCP2JJ2sF29MnTyVzd+2NFR/wOuybZlK0=; b=rufkC4vQvU5bIxmK6VYCz02Mb3Unmpd31EBOkJ+fEzgZ2Q/qMSmwRRbEhgau4BH7U/ Lq4FnRCVz5hrlrHAHBD+JfQP48MssX70TGiQolcCORQJOzTdHdUo5ZOhxm7eeV1Kzf84 jfZWOog7nMglUmwDhN94UNZF4QKHHqAhWTMMZ8QsYkS3TU8MezyEIsD+xOWYmK9r9j2P G8pPZ7e92DgT6LOgbRWM0jdPRe71wHangEf6DsGClT8ZZ3dkzm04m8LYnj7umTLT2FmS gFjOrGTBdAr6Sk6JgYLzqLCtpHCu2y/fl0N/gbr775HflaSGBmFtvX/lCklI/O76753X ho0w== X-Gm-Message-State: AOAM530+8cQXAyMvDKRvHIFOazVxQxVwdw/NwbtxcgQEUh17Pnb3Uog6 D1/06my2Logj4CGSd++YZ98pK5wrzCcrXw== X-Google-Smtp-Source: ABdhPJyk9KPhG8BEcFLyIyZGlLp3d6iXrDNVXB0di6ROlZ84YTeOrLAntpSXQMr5jlKpNftqhZQ91w== X-Received: by 2002:a17:907:2149:: with SMTP id rk9mr29545155ejb.553.1595428423956; Wed, 22 Jul 2020 07:33:43 -0700 (PDT) Received: from gmail.com ([2a03:1b20:3:f011::6d]) by smtp.gmail.com with ESMTPSA id q3sm34992eds.41.2020.07.22.07.33.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jul 2020 07:33:43 -0700 (PDT) Date: Wed, 22 Jul 2020 16:33:45 +0200 From: Shahab Vahedi To: Simon Marchi Cc: gdb-patches@sourceware.org, Shahab Vahedi , Tom Tromey , Anton Kolesov , Francois Bedard Subject: Re: [PATCH v3 1/3] arc: Add ARCv2 XML target along with refactoring Message-ID: <20200722143345.GC10630@gmail.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0731a886-0896-0776-11a9-94f44810eb75@simark.ca> X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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:33:47 -0000 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); } 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); ... } Cheers, Shahab