From patchwork Fri Nov 18 15:05:47 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Arnez X-Patchwork-Id: 17562 Received: (qmail 15145 invoked by alias); 18 Nov 2016 15:06:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 15132 invoked by uid 89); 18 Nov 2016 15:06:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW, RCVD_IN_SEMBACKSCATTER autolearn=no version=3.3.2 spammy=1string, 2.3.0, compose, threw X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 Nov 2016 15:05:56 +0000 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAIEwXb7103790 for ; Fri, 18 Nov 2016 10:05:54 -0500 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0a-001b2d01.pphosted.com with ESMTP id 26t39xa5n3-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 18 Nov 2016 10:05:54 -0500 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 18 Nov 2016 15:05:52 -0000 Received: from d06dlp02.portsmouth.uk.ibm.com (9.149.20.14) by e06smtp11.uk.ibm.com (192.168.101.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 18 Nov 2016 15:05:50 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 36DA52190056 for ; Fri, 18 Nov 2016 15:05:03 +0000 (GMT) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id uAIF5ohX34144434 for ; Fri, 18 Nov 2016 15:05:50 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id uAIF5nTm002852 for ; Fri, 18 Nov 2016 08:05:50 -0700 Received: from oc1027705133.ibm.com (dyn-9-152-212-182.boeblingen.de.ibm.com [9.152.212.182]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id uAIF5lT2002837 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 18 Nov 2016 08:05:49 -0700 From: Andreas Arnez To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] Fix copy_bitwise() References: <1479135786-31150-1-git-send-email-arnez@linux.vnet.ibm.com> <1479135786-31150-3-git-send-email-arnez@linux.vnet.ibm.com> <6032efa2-828d-7423-4720-6925a9b4ea4b@redhat.com> Date: Fri, 18 Nov 2016 16:05:47 +0100 In-Reply-To: (Pedro Alves's message of "Thu, 17 Nov 2016 20:30:45 +0000") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16111815-0040-0000-0000-00000308853F X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16111815-0041-0000-0000-00001DEB14AA Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-11-18_09:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611180258 X-IsSubscribed: yes On Thu, Nov 17 2016, Pedro Alves wrote: > I'm feeling dense and I can't make sense of the new test. :-/ > > Can you add some more comments? Is there some logic behind the > numbers of the data1/data2 arrays? Some pattern between them? > E.g., it'd be nice to explain the logic between the steps > check_copy_bitwise is doing. OK, commented the array definitions and the steps of check_copy_bitwise. Also gave the variables a, b, and data1/2 more telling names. > A couple other minor comments: > > >> +#if GDB_SELF_TEST >> + > > Please add: > > namespace selftests { > > like utils-selftests.c does. Done. >> + a[len] = b[len] = '\0'; >> + if (strcmp (a, b)) > > strcmp (...) != 0 Done. >> + error (_("copy_bitwise %s != %s (%u+%u -> %u)"), >> + a, b, source_offset, nbits, dest_offset); >> +} > >> + >> + enum { max_nbits = 24, max_offs = 8 }; > > IN C++11 we can express a compile-time integer directly with > constexpr, not need for enum hacks any longer: > > constexpr int max_nbits = 24; Done. Updated patch below. Does this help? --- Andreas -- >8 -- Subject: [PATCH v2] Add unit test for copy_bitwise This adds a unit test for the copy_bitwise function in dwarf2loc.c. With the old (broken) version of copy_bitwise this test would generate the following failure message: (gdb) maintenance selftest Self test failed: copy_bitwise 1101110111010110 != 1101110110010110 (0+9 -> 1) Ran 3 unit tests, 1 failed gdb/ChangeLog: * dwarf2loc.c (bits_to_str, check_copy_bitwise) (copy_bitwise_tests): New functions. (_initialize_dwarf2loc): Register the new function copy_bitwise_tests as a unit test. * selftest.c (run_self_tests): Improve the failure message's wording and formatting. --- gdb/dwarf2loc.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/selftest.c | 3 +- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c index b238133..c7e0380 100644 --- a/gdb/dwarf2loc.c +++ b/gdb/dwarf2loc.c @@ -38,6 +38,7 @@ #include "dwarf2loc.h" #include "dwarf2-frame.h" #include "compile/compile.h" +#include "selftest.h" #include #include @@ -1567,6 +1568,101 @@ copy_bitwise (gdb_byte *dest, ULONGEST dest_offset, } } +#if GDB_SELF_TEST + +namespace selftests { + +/* Helper function for the unit test of copy_bitwise. Convert NBITS bits + out of BITS, starting at OFFS, to the respective '0'/'1'-string. MSB0 + specifies whether to assume big endian bit numbering. Store the + resulting (not null-terminated) string at STR. */ + +static void +bits_to_str (char *str, const gdb_byte *bits, ULONGEST offs, + ULONGEST nbits, int msb0) +{ + unsigned int j; + size_t i; + + for (i = offs / 8, j = offs % 8; nbits; i++, j = 0) + { + unsigned int ch = bits[i]; + for (; j < 8 && nbits; j++, nbits--) + *str++ = (ch & (msb0 ? (1 << (7 - j)) : (1 << j))) ? '1' : '0'; + } +} + +/* Check one invocation of copy_bitwise with the given parameters. */ + +static void +check_copy_bitwise (const gdb_byte *dest, unsigned int dest_offset, + const gdb_byte *source, unsigned int source_offset, + unsigned int nbits, int msb0) +{ + size_t len = (dest_offset + nbits + 7) / 8 * 8; + char *expected = (char *) alloca (len + 1); + char *actual = (char *) alloca (len + 1); + gdb_byte *buf = (gdb_byte *) alloca (len / 8); + + /* Compose a '0'/'1'-string that represents the expected result of + copy_bitwise. */ + bits_to_str (expected, dest, 0, len, msb0); + bits_to_str (expected + dest_offset, source, source_offset, nbits, msb0); + + /* Fill BUF with data from DEST, apply copy_bitwise, and convert the + result to a '0'/'1'-string. */ + memcpy (buf, dest, len / 8); + copy_bitwise (buf, dest_offset, source, source_offset, nbits, msb0); + bits_to_str (actual, buf, 0, len, msb0); + + /* Compare the resulting strings. */ + expected[len] = actual[len] = '\0'; + if (strcmp (expected, actual) != 0) + error (_("copy_bitwise %s != %s (%u+%u -> %u)"), + expected, actual, source_offset, nbits, dest_offset); +} + +/* Unit test for copy_bitwise. */ + +static void +copy_bitwise_tests (void) +{ + /* Some random data, to be used for source and target buffers. */ + static const gdb_byte source_data[] = + { 0xdd, 0x85, 0x51, 0x93, 0xb3, 0xc0, 0x62, 0xd4, + 0xa6, 0x4b, 0x0a, 0xde, 0x36, 0x35, 0x1a, 0x66 }; + + static const gdb_byte dest_data[] = + { 0x47, 0x6a, 0x65, 0x61, 0xab, 0xed, 0xb6, 0xa9, + 0xbf, 0x67, 0xb2, 0xeb, 0x86, 0x45, 0xfe, 0x09 }; + + constexpr int max_nbits = 24; + constexpr int max_offs = 8; + unsigned int from_offs, nbits, to_offs; + int msb0; + + for (msb0 = 0; msb0 < 2; msb0++) + { + for (from_offs = 0; from_offs <= max_offs; from_offs++) + { + for (nbits = 1; nbits < max_nbits; nbits++) + { + for (to_offs = 0; to_offs <= max_offs; to_offs++) + check_copy_bitwise (dest_data, to_offs, + source_data, from_offs, nbits, msb0); + } + } + /* Special cases: copy all, copy nothing. */ + check_copy_bitwise (dest_data, 0, source_data, 0, + sizeof (source_data) * 8, msb0); + check_copy_bitwise (dest_data, 7, source_data, 9, 0, msb0); + } +} + +} /* namespace selftests */ + +#endif /* GDB_SELF_TEST */ + static void read_pieced_value (struct value *v) { @@ -4527,4 +4623,8 @@ _initialize_dwarf2loc (void) NULL, show_entry_values_debug, &setdebuglist, &showdebuglist); + +#if GDB_SELF_TEST + register_self_test (selftests::copy_bitwise_tests); +#endif } diff --git a/gdb/selftest.c b/gdb/selftest.c index 110b9e5..f771e57 100644 --- a/gdb/selftest.c +++ b/gdb/selftest.c @@ -50,8 +50,7 @@ run_self_tests (void) CATCH (ex, RETURN_MASK_ERROR) { ++failed; - exception_fprintf (gdb_stderr, ex, - _("Self test threw exception")); + exception_fprintf (gdb_stderr, ex, _("Self test failed: ")); } END_CATCH }