From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 63091 invoked by alias); 9 Oct 2017 09:11:29 -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 63079 invoked by uid 89); 9 Oct 2017 09:11:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=Changes, handy, initially, H*c:alternative X-HELO: EUR02-AM5-obe.outbound.protection.outlook.com Received: from mail-eopbgr00130.outbound.protection.outlook.com (HELO EUR02-AM5-obe.outbound.protection.outlook.com) (40.107.0.130) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 09:11:26 +0000 Received: from VI1PR0501MB2861.eurprd05.prod.outlook.com (10.172.11.151) by VI1PR0501MB2864.eurprd05.prod.outlook.com (10.172.12.9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.77.7; Mon, 9 Oct 2017 09:11:21 +0000 Received: from VI1PR0501MB2861.eurprd05.prod.outlook.com ([fe80::1105:ec3d:d331:ab8e]) by VI1PR0501MB2861.eurprd05.prod.outlook.com ([fe80::1105:ec3d:d331:ab8e%16]) with mapi id 15.20.0077.016; Mon, 9 Oct 2017 09:11:21 +0000 From: Peeter Joot To: Simon Marchi , "gdb-patches@sourceware.org" Subject: Re: review request: implementing DW_AT_endianity Date: Mon, 09 Oct 2017 09:11:00 -0000 Message-ID: References: ,<0f87cf42-781d-4d70-a389-f4688cde109d@simark.ca> In-Reply-To: <0f87cf42-781d-4d70-a389-f4688cde109d@simark.ca> authentication-results: spf=none (sender IP is ) smtp.mailfrom=peeter.joot@lzlabs.com; x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;VI1PR0501MB2864;6:efhj5JOxlnipxIG2QaAzKa0ahtmArC+AMnPQmW2+iGMMrXNOUlcegpM4/0ndeJV7LBsyMuRo0OX0GJjDAyvr1bJaiSfzD+W6A1JkrbKRwsDTOlQpxHt8IpuAYqS5ueECvU8FKjIx3E0UKL42bGuW1VRzBVlngiZrlVPQYE7xflLxEAxUvGBv9T6OjSyPomCsygbpvfoOxZdAgR45RLXLEp4KGEQRVOh5DPpHWmjffH+LwxRiQ5ReiohJW1VqGWG2SuRgOVNgwTDs+sA8g9Mf7qzUrflYCDtIcIj2mBAAAyD0kpXkMTzOsadXZ0mRmViVxr5KzWaOEmeqnlzdaDPW5g==;5:XllGNY6/ZdApSfO8k7GMigKR4r+e0H2k5JfI+7LYQ50pR2aJmQ9IdXWO7jGCYFmMqhqobFNsdKDQMcnN6qOJUNoKH9MxbDtlidOL+TDEMelr6AaBAHrVb1aM8n/TgnDdO/fXLzd1DxoK7+xQNPdC3M+K+e9+ihFc7U8psXDbLZA=;24:630UHaICGPDC1qwtfjyzS1efPdPgsEwkHk5ffYFu0K4vh4tQ6lvc3iFUVLlZb9JIBqIjwEYsHyz2jhudtqf3o9IcG/ZC73LVKLy5lahtjKM=;7:1/ui4nyHZKVeoqV1YD1PVyefiAX9wQUutTdJwhcR11PQ7mPRXLI7ygNn4ZLMpOtaRtDjtOx07Po1MCh5ohs4zSthSLI1FSOSUYCy+vgE+ogm9c2ifDuo16DatGKgJm7JxOW7PmBd7/JcCfF9hPTUQ91A1DaT5suefqCEe0XCkYkvEfftg7mPFOiB0KkYMhO8bqTg+KBoXjjSsQgyJcWISsNkmjRMjEpodVRLjpwgLqc= x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 590ba5bb-9cb8-4d26-6f76-08d50ef5b596 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:VI1PR0501MB2864; x-ms-traffictypediagnostic: VI1PR0501MB2864: x-exchange-antispam-report-test: UriScan:; x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(3002001)(10201501046)(100000703101)(100105400095)(93006095)(93001095)(6041248)(20161123558100)(20161123560025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:VI1PR0501MB2864;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:VI1PR0501MB2864; x-forefront-prvs: 045584D28C x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(346002)(39830400002)(376002)(199003)(51914003)(189002)(6306002)(6506006)(106356001)(101416001)(6116002)(102836003)(55016002)(25786009)(99286003)(74316002)(68736007)(7696004)(7736002)(33656002)(50986999)(76176999)(54356999)(316002)(86362001)(14454004)(5660300001)(2906002)(2950100002)(189998001)(2501003)(8676002)(81166006)(6436002)(97736004)(81156014)(966005)(53936002)(6606003)(110136005)(5250100002)(8936002)(229853002)(6246003)(19627405001)(54896002)(3660700001)(3280700002)(478600001)(105586002)(2900100001)(9686003);DIR:OUT;SFP:1102;SCL:1;SRVR:VI1PR0501MB2864;H:VI1PR0501MB2861.eurprd05.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; received-spf: None (protection.outlook.com: lzlabs.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: lzlabs.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Oct 2017 09:11:21.7542 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: be5a2f53-aa84-427c-8fbd-c2d71558a7a8 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0501MB2864 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2017-10/txt/msg00188.txt.bz2 Hi Simon, > Please read the Contribution Checklist: > > https://sourceware.org/gdb/wiki/ContributionChecklist > > Most importantly, make sure your code uses the GNU style, and use "git se= nd-email" to > send your patch. It will make it easier for reviewers to apply and look = at your patch. > > Changes to binutils should be sent the binutils mailing list (binutils@so= urceware.org), so > make a separate patch for binutils/dwarf.c and send it there. > > The best way to show that your contribution works is to add a test for it= . You can add it > to testsuite/gdb.base. Copy an existing one (e.g. wchar.c/wchar.exp) and= modify as needed. > > You should also run the testsuite to see if your patch causes any regress= ion: > > https://sourceware.org/gdb/wiki/TestingGDB > > Note that there are many existing failures in the testsuite, so what you = should do is run the > testsuite without and with your patch, and diff the before/after testsuit= e/gdb.sum file. Thanks for the new-developer tips. > > --- a/binutils/dwarf.c > > +++ b/binutils/dwarf.c ... > Put each statement on its own line: > > case DW_END_default: > printf ("(default)"); > break; Note that if I did this, the addition wouldn't match any of the case statem= ents in the surrounding code (read_and_display_attr_value). There are case= statements matching your suggested style earlier in this function, but oth= ers that do not (like all the ones immediately surrounding my addition). I= 've temporarily adjusted my addition to match the immediately surrounding c= ode (i.e. using tabs instead of spaces, which I hadn't initially noticed). = Shall I adjust all of that surrounding "non-standard formatting" so that e= ach statement is on its own line, or be consistent locally with the convent= ions used in this file? I've fixed up all the other formatting issues that you've pointed out, and = will submit a new patch with git send-email after running the test suite, a= nd adding my own new test. The next patch I send will have an additional m= echanical change, altering blocks of code from: gdbarch_byte_order (get_type_arch (type1)) to: gdbarch_byte_order_for_type (NULL, type1) where the following helper function has been added to gdbtypes.c: enum bfd_endian gdbarch_byte_order_for_type (struct gdbarch * gdbarch, struct type *type) { if (TYPE_ENDIANITY_BIG (type)) return BFD_ENDIAN_BIG; else if (TYPE_ENDIANITY_LITTLE (type)) return BFD_ENDIAN_LITTLE; if (!gdbarch) gdbarch =3D get_type_arch (type); return gdbarch_byte_order (gdbarch); } When the caller of this already had a gdbarch variable handy, I've used tha= t, instead of passing NULL. This transformation is enough to make gdb assi= gnment to a mixed endian field, as in: (gdb) p b.v =3D 4 work properly, storing the value in big-endian format. This is the using t= he same example from my original review request email, assuming a little en= dian target and a structure with a big-endian attribute. In the process of doing this debug testing, I've noticed that gcc appears t= o have some bugs with respect to its dwarf endianity attribute tagging. Fo= r example: #include #include typedef int int32be_t; typedef short int16be_t; struct big { int32be_t v; int16be_t a[4]; } __attribute__( ( scalar_storage_order( "big-endian" ) ) ); struct native { int v; short a[4]; }; int main() { struct big b =3D {3, {1, 2, 3, 4}}; struct native n =3D {3, {1, 2, 3, 4}}; printf( "%d\n", b.v ); printf( "%d\n", n.v ); return 0; } A version of this code that does not use typedefs for the field types does = get tagged with DW_AT_endianity/DW_END_big, but when typedefs are used as a= bove, the object code ends up with no endianity attributes at all (although= the compiler does insert the desired bswap operations). It appears that i= t may take a few iterations of compiler/debugger enhancements to really get= this working nicely together. There is a bigger issue of the DW_AT_name that gets emitted for a field wit= h non-native endianity. I think it would be best for the compiler to choos= e a different DW_AT_name than the default (int.be for example). If that is= not done it looks like it breaks gdb's assumption that there is one set of= attributes for each type in question. This could be considered a gdb bug,= but I think it would be better for the compiler to cater to gdb in this ca= se. I'm curious what other developers (especially anybody that works on bo= th gcc and gdb) think about this. > I would consider a change like that to be significant enough to require a= n assignment. Okay. I've contacted the FSF assignment representative to start this proce= ss. Peeter