From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 44538 invoked by alias); 18 Sep 2018 11:55:55 -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 41234 invoked by uid 89); 18 Sep 2018 11:55:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=HTo:U*rth X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Sep 2018 11:55:53 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1EA4300289E; Tue, 18 Sep 2018 11:55:51 +0000 (UTC) Received: from [10.36.117.118] (ovpn-117-118.ams2.redhat.com [10.36.117.118]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B42F65D973; Tue, 18 Sep 2018 11:55:49 +0000 (UTC) To: Stafford Horne , Richard Henderson Cc: binutils@sourceware.org, GDB patches , Openrisc References: <20180821143823.16985-1-shorne@gmail.com> <20180908213515.GN4594@lianli.shorne-pla.net> <20180918095234.GP4594@lianli.shorne-pla.net> From: Nick Clifton Openpgp: preference=signencrypt Subject: Re: [PATCH 0/4] OpenRISC binutils updates and new relocs Message-ID: <20b65fc3-5bbb-6e77-f598-4582204ee0e5@redhat.com> Date: Tue, 18 Sep 2018 11:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180918095234.GP4594@lianli.shorne-pla.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-09/txt/msg00626.txt.bz2 Hi Stafford, >> There are a few minor formatting glitches, but nothing serious. > > Will you be able to point them out? Even just some hints? Sure... So for example in patch 1/4 there is: +enum { + RTYPE_LO = 0, when really it should be: +enum +{ + RTYPE_LO = 0, (And similarly in other places. Basically, try to avoid ending a line with an opening curly brace, unless that brace is the only character on the line). Then there is: +static int +parse_reloc(const char **strp) Which ought to be: +static int +parse_reloc (const char **strp) Ie - a single space between the function name and its parameters. (I did say that these were minor formatting nits...) In a similar vein there is: + return parse_imm16(cd, strp, opindex, (long *) valuep, 0); +} which also needs a space between the function name and its arguments. There are a few other cases of the above issues in the other patches, but nothing else of note. One other thing: There are several places where you add calls to abort(). Now this is not wrong, and certainly not a reason to reject the patch, but I consider it to be unhelpful. To my mind a library, or tool, should generate an error message when something goes wrong and not leave the user wondering why they have suddenly got a segmentation fault. Plus if you have a call to abort() in the code you can bet that some enterprising person with a binary fuzzer will find a way to trigger it, and then file a CVE about it. (Fixing CVEs is the bane of my life as they involve lots of extra administrivia). >> I do not see any need to add extra document for the new relocs, unless you >> have created new assembler pseudo-ops to generate them. > As Richard mentioned we have added a few, see PATCH 3/4 in cpu/or1k.opc the > change: > > (parse_reloc): Add new po(), gotpo() and gottppo() for LO13 relocations. > > Is this what you mean? I will look into adding the documentation for these. Please do. Most likely you will want to create a gas/doc/c-or1k.c file, (copying the contents from another, similar file and modifying as needed), and then patch the gas/doc/as.texi file to include it and the gas/doc/all.texi file to define a macro for it. >> I do have one question though. Is there a need to be able to distinguish >> between binaries that use the new l.adrp instruction and those that don't. > As Richard mentioned we don't handle this. > > We have cases like this right now as well, i.e. binaries generated with `l.mul` > or `l.div` instructions will link fine into an executable that assume those > instrunctions should be emulated. That doesn't throw an error and I don't think > it has been a problem. OK, well it is your target, so if you are OK with this then so be it. I would recommend however thinking about a solution for the future, should the openRISC architecture gain more variants. My suggestion would be to make use of ELF notes, as has been done with other ports. Cheers Nick