From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76964 invoked by alias); 26 May 2017 10:26:47 -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 76948 invoked by uid 89); 26 May 2017 10:26:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=beef X-HELO: mail-wm0-f54.google.com Received: from mail-wm0-f54.google.com (HELO mail-wm0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 26 May 2017 10:26:45 +0000 Received: by mail-wm0-f54.google.com with SMTP id d127so16494627wmf.0 for ; Fri, 26 May 2017 03:26:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=gzYUQRMuCDn+2FjcBefUUHa5RepCLJGEFg0zKfRgkHw=; b=iLRl6trCI21svw6795Y9Z9CQSKPPV5qddf1jWX2WW8v1Fybq6XyQzi5yiZi6xQkg8j UTpF8VuyLOkfLRe68bMxgN4GLJKo1cmW0uvPcwxs15zShmArl66xDgvt7YJ3TOniDEiB ebiTqlk7L7uwddmh0nSSkDLM9sNa6nSSpsqyobYRJKmZxnB/P20jQRkDwkG7RP2SCzR/ UDvSK7JpGpWHd0f/F+us18jKdqM3AV62hZSMi/v+1ACjbxNnCOk/AyHZU0DnQvw0sEWt c5N7rnWACEqlTeIF+uOZSZd3zhbFgUNuAIjzgSH4uY4DhktaGeNk75gd756R8pYtMeKf L2XA== X-Gm-Message-State: AODbwcA+LrhWxFo+hKIyUoAiohEbPnJUO+Kk5VA0wcLPtRzpSDz60aHj 2qArywNQ3xG/0y76 X-Received: by 10.28.69.143 with SMTP id l15mr1726175wmi.116.1495794406796; Fri, 26 May 2017 03:26:46 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id f78sm9268226wme.19.2017.05.26.03.26.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 26 May 2017 03:26:45 -0700 (PDT) Subject: Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4) To: Alan Hayward References: <3C00280E-37C9-4C0A-9DA6-F3B9DB1A6E8F@arm.com> <86y3v7uf9j.fsf@gmail.com> <806B436F-EFA1-4200-AC54-9036D166C9B9@arm.com> <867f1m8nhm.fsf@gmail.com> <8637bx9jsw.fsf@gmail.com> <78A7E8EA-7203-44DF-B7FD-63E75A5ECEF5@arm.com> <540372d8-efc3-f842-5cac-cd813bacc3f5@redhat.com> <4F90CD36-759D-4BDA-BFEC-8DD86F44A0B7@arm.com> <40597975-9458-e9af-8915-9d303bb1ed98@redhat.com> <5A105765-C70D-413C-BB35-50BAA5FD5865@arm.com> <73b5b4f8-065b-7102-a9d8-0b909b1eb124@redhat.com> <86wp95p3ev.fsf@gmail.com> <2b6c26fe-be7e-e219-7c6a-7fb7425f7dc1@redhat.com> Cc: Yao Qi , "gdb-patches@sourceware.org" , nd From: Pedro Alves Message-ID: <73867a49-889b-171d-db60-d545da68b8dd@redhat.com> Date: Fri, 26 May 2017 10:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00556.txt.bz2 On 05/26/2017 09:54 AM, Alan Hayward wrote: > 2017-05-25 Alan Hayward > > * defs.h (copy_integer_to_size): New declaration. > * findvar.c (copy_integer_to_size): New function. > (do_cuint_test): New selftest function. > (do_cint_test): New selftest function. > (copy_integer_to_size_test): Likewise. > (_initialize_findvar): Likewise. > * mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer. > (mips_fbsd_collect_reg): Use raw_collect_integer. > * mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer. > (mips64_fill_gregset): Use raw_collect_integer > (mips64_fill_fpregset): Use raw_supply_integer. > * regcache.c (regcache::raw_supply_integer): New function. > (regcache::raw_collect_integer): Likewise. > * regcache.h: (regcache::raw_supply_integer): New declaration. > (regcache::raw_collect_integer): Likewise. Last line looks like needs s/spaces/tab/. > +#if GDB_SELF_TEST > +namespace selftests { > +namespace findvar_tests { > + > +/* Functions to test copy_integer_to_size. Store SOURCE_VAL with size > + SOURCE_SIZE to a buffer, making sure no sign extending happens at this > + stage. Copy buffer to a new buffer using copy_integer_to_size. Extract > + copied value and compare to DEST_VAL. Do all of this for both LITTLE and > + BIG target endians. */ > + > +static void > +do_cuint_test (long dest_val, int dest_size, ULONGEST src_val, int src_size) > +{ > + for (int i = 0; i < 2 ; i++) > + { > + ULONGEST srcbuf_val = 0, destbuf_val = 0; > + gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val; > + gdb_byte* destbuf = (gdb_byte*) &destbuf_val; Nit, I'd prefer writing instead: gdb_byte srcbuf[sizeof (ULONGEST)] = {}; gdb_byte destbuf[sizeof (ULONGEST)] = {}; I.e., get rid of the local ULONGEST variables. It took me a second to realize that below you're writing to these variables instead of to "dest_val" directly, which made me first wonder about strict aliasing issues. (But see further below.) > +static void > +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size) > +{ > + for (int i = 0; i < 2 ; i++) > + { > + LONGEST srcbuf_val = 0, destbuf_val = 0; > + gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val; > + gdb_byte* destbuf = (gdb_byte*) &destbuf_val; Ditto. > + enum bfd_endian byte_order = i ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG; > + > + store_unsigned_integer (srcbuf, src_size, byte_order, src_val); > + copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true, > + byte_order); > + SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size, > + byte_order)); > + } > +} > + > +static void > +copy_integer_to_size_test () > +{ ... > + do_cint_test (0xffffffffdeadbeef, 8, 0xdeadbeef, 4); > + do_cint_test (0xffffffffffadbeef, 8, 0xdeadbeef, 3); > + do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3); Note that: > +static void > +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size) "long" can be 32-bit, depending on host. It's 32-bit on 64-bit Windows. So you're passing 0xffffffffffffbeef, which gets truncated to 0xffffbeef, but then sign extended here again: SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size, byte_order)); and it'll work out (by chance). I think it would be better to always pass an unsigned ULONGEST as dest_val: static void do_cint_test (ULONGEST dest_val, int dest_size, LONGEST src_val, int src_size) likewise for do_cuint_test, of course. Makes me notice that you didn't add any unsigned test for src/dest size > 4, which would fail on 64-bit Windows, for the same reason (32-bit long), like: do_cuint_test (0xff12345678, 8, 0xff12345678, 6); I notice you have no tests that exercise src_size == dest_size, for multiple sizes. It'd be good to add them. Another issue with the test is that by relying on extract_(un)signed_integer, the test can't notice if the zero / sign extension in copy_integer_bytes filled the right number of bytes: > + do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3); I.e., above, the last test _must_ not write more than two bytes to the destination, so those leading 0xFFs before "beef" are artificial. Passing ULONGEST as destination value avoids that problem, making you write that last test as: do_cint_test (0x000000000000beef, 2, 0xbeef, 3); That brings us to the next problem, that you can't tell whether zero extension writes the zeros in the first place. I.e., here: do_cuint_test (0x12345678, 8, 0x12345678, 4); Given do_cuint_test does this: + ULONGEST srcbuf_val = 0, destbuf_val = 0; You can't tell whether copy_integer_to_size really filled in bytes 4-8 with zero. You can get that back by making the do_cint_test/do_int_test functions memset the temporary buffers with some byte that the callers never pass down as part of any src/dest value. Like, say reserve 0xaa for that: gdb_byte srcbuf[sizeof (ULONGEST)]; gdb_byte destbuf[sizeof (ULONGEST)]; memset (srcbuf, 0xaa, sizeof (srcbuf)); memset (destbuf, 0xaa, sizeof (srcbuf)); (with suitable comments) So in sum: #1 - ULONGEST as destination value, both signed and unsigned tests. #2 - Add unsigned tests with size >4 #3 - Add tests src_size == dest_size #4 - Use gdb_byte arrays for temporary buffers, and memset them with 0xaa. #5 - Adjust tests to pass. #6 - Add comments where appropriate. Thanks, Pedro Alves