From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x544.google.com (mail-ed1-x544.google.com [IPv6:2a00:1450:4864:20::544]) by sourceware.org (Postfix) with ESMTPS id F1064384240C for ; Wed, 15 Jul 2020 20:35:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F1064384240C Received: by mail-ed1-x544.google.com with SMTP id g20so2697154edm.4 for ; Wed, 15 Jul 2020 13:35:02 -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-description:content-disposition:in-reply-to; bh=/bzThEZBmI3eTsPlhz7A+7B716mLj9xGfW4GhLNgXLQ=; b=Pot8RFTqsV1NSj1hjpw6J59CQAYXKblqBe50vwL6Wyu7dfBSBnzbpuhJVCMr6Yxo1v L28JBsg5bDuv6yoMhxQo35nmksaF0/di10sc/ccnxM4R30v2ZM5nhb/t+QAFYzyx3cOq zCTB39uGGoYKVzh4EeZrWqs1AmDOkdeSjMD16JMBb/SucAhn1TYXYyzZvjTR4P1C5TuO G7oLt7ODfDvLP5fzfsbR+i+gPwJczp/vxZV4cPuNeKMqgUAatZASTg6fHnCyBm85GvA1 bI4fSSYVzatH356EEoSkElX+JnuGnTiSJgC2DDoYISKii/c+5cVbOKUsROeBK9su0Mrg 603g== X-Gm-Message-State: AOAM530TAWz4wG2YbEEikpjYiSVMm9c8N5k9M1opwkbh02EX8f+aC6p9 mMC8/139H+zSLor2B3REa5g= X-Google-Smtp-Source: ABdhPJyKjzSHzMQh9okEjcpas37KdxAtYSkp11LDN9jL9KyYr6eLQ4d+TtoXc8CI47tW5lBe1QYbvQ== X-Received: by 2002:aa7:c545:: with SMTP id s5mr1355387edr.19.1594845301955; Wed, 15 Jul 2020 13:35:01 -0700 (PDT) Received: from gmail.com ([2a03:1b20:3:f011::6d]) by smtp.gmail.com with ESMTPSA id o18sm3012880ejr.45.2020.07.15.13.35.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jul 2020 13:35:00 -0700 (PDT) Date: Wed, 15 Jul 2020 22:35:00 +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: <20200715203500.GA4811@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Description: v3_response_to_simon Content-Disposition: inline In-Reply-To: <422b0905-456f-8722-b74d-97ade4a95c58@simark.ca> X-Spam-Status: No, score=-7.3 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, 15 Jul 2020 20:35:04 -0000 Hi Simon, Thank you a lot for going through all the changes. I've taken most of your remarks in. There are a few of them that need answering. Below, you will find them. If you think the current state of code is satisfactory, I will go ahead and merge them. With one note: there might come another patch on top of this if the output of "AddressSanitizer" shows some leak. Please see my question for the "AddressSanitizer" below. On Tue, Jul 14, 2020 at 10:52:12PM -0400, Simon Marchi wrote: > Hi Shahab, > > > +/* Common construction code for ARC_GDBARCH_FEATURES struct. If there > > + is no ABFD, then a FEATURE with default values is returned. */ > > +void arc_gdbarch_features_init (arc_gdbarch_features &features, > > + const bfd *abfd, const unsigned long mach) > > Since this function is only used in this file, can it be static? The other patch [1] in this series makes use of this function. [1] [PATCH v3 3/3] arc: Add GNU/Linux support for ARC https://sourceware.org/pipermail/gdb-patches/2020-July/170429.html > Could this function return a arc_gdbarch_features by value instead? Then, > you could add a constructor to arc_gdbarch_features. You wouldn't need > the "A 0 indicates an uninitialised value" comment on reg_size, as the > object could never be in an uninitialised state. Those were my thoughts as well. I tried something like that, but I failed in the end: Since the "arc_gdbarch_features" is introduced in "gdb/arch/arc.h", I tried implementing the constructor in "gdb/arch/arc.c". Because the constructor had to make use of "struct bfd", "bfd_get_flavour ()", "elf_header ()", "EFCLASS*", etc. the following headers had to be used in "gdb/arch/arc.c": #include "defs.h" #include "arch-utils.h" #include "elf-bfd.h" This would build OK for gdb targets, but when compiling "arc.o" for gdbserver target, I ended up with something like: (paraphrasing) Error: defs.h should not be included while building gdbserver. (I am not sure if the error was indeed about "defs.h") And removing/if-def-guarding that header file was opening other cans of worms. Eventually, I gave up and went for the current approach. > > +/* Wrapper used by std::unordered_map to generate hash for features set. */ > > +struct arc_gdbarch_features_hasher > > +{ > > + std::size_t > > + operator() (const arc_gdbarch_features &features) const noexcept > > + { > > + return features.hash (); > > + } > > +}; > > + > > +/* Cache of previously created target descriptions, indexed by the hash > > + of the features set used to create them. */ > > +static std::unordered_map > + const target_desc *, > > + arc_gdbarch_features_hasher> arc_tdesc_cache; > > Should this map hold unique_ptr of target_desc? I understand that they > are never really deallocated anyway, but just to do it right and avoid > adding more leaks to the final AddressSanitizer report shown when you exit > GDB (and you have it build with AddressSanitizer)? No, I haven't built it with AddressSanitizer. Could you point me in the right direction how I can use it? > > + /* Used by std::unordered_map to hash the feature sets. The hash is > > + calculated in the manner below: > > + REG_SIZE | ISA > > + 5-bits | 4-bits */ > > + > > + std::size_t hash () const noexcept > > + { > > + std::size_t val = ((reg_size & 0x1f) << 8 | (isa & 0xf) << 0); > > + return val; > > I don't know much about hashing, but I don't think makes for a very good hash > function. It outputs similar hashes for similar inputs, and does not use the > range of possible hash values uniformly. Indeed it does not output hashes uniformly but I believe it is unique. The meaningful (and future proof) values for "reg_size" are {4, 8, 16} _bytes_. Hence, the "0x1f" mask should suffice. Having 15 versions for ISA should be enough as well. Having these conditions in mind, I don't think there is any collusion while hashing such inputs. > It would be more appropriate and typical to use std::hash on each member and > then combine the hashes. Doing a quick search I found that Boost offers > something exactly for that called hash_combine: > > https://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html Thanks for this. I didn't know about such functionality. As you said though, we can't use it. Personally, I think it would be an overkill here. We can get away with a simple "shift" and "or". Please read my response above for my reasoning. > > Unfortunately, there is nothing similar in the standard library. But > > A bit off-topic: intuitively, I would have thought that it would have been > sufficient to just add the hash values together. Anyone knows why this > wouldn't be a good idea? So far, possible values are: reg_size : {4, 8, 16} ISA: {1, 2, ... 15} A simple addition would produce the same result for these legally valid inputs: reg_size=8, ISA=2 ---> hash = 10 reg_size=4, ISA=6 ---> hash = 10 Shahab