From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30237 invoked by alias); 18 Sep 2018 12:08:06 -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 29665 invoked by uid 89); 18 Sep 2018 12:08:05 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,HTML_MESSAGE,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 spammy=Plus, H*c:alternative X-HELO: mx.coeval.ca Received: from mx.coeval.ca (HELO mx.coeval.ca) (184.75.211.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Sep 2018 12:08:02 +0000 Received: from mail-ot1-f47.google.com (mail-ot1-f47.google.com [209.85.210.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx.coeval.ca (Postfix) with ESMTPSA id EFDF1436054; Tue, 18 Sep 2018 12:08:00 +0000 (UTC) Received: by mail-ot1-f47.google.com with SMTP id n5-v6so1691366otl.5; Tue, 18 Sep 2018 05:08:00 -0700 (PDT) MIME-Version: 1.0 References: <20180821143823.16985-1-shorne@gmail.com> <20180908213515.GN4594@lianli.shorne-pla.net> <20180918095234.GP4594@lianli.shorne-pla.net> <20b65fc3-5bbb-6e77-f598-4582204ee0e5@redhat.com> In-Reply-To: <20b65fc3-5bbb-6e77-f598-4582204ee0e5@redhat.com> Reply-To: joel@rtems.org From: Joel Sherrill Date: Tue, 18 Sep 2018 12:08:00 -0000 Message-ID: Subject: Re: [PATCH 0/4] OpenRISC binutils updates and new relocs To: Nick Clifton Cc: Stafford Horne , Richard Henderson , binutils@sourceware.org, GDB patches , Openrisc Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-09/txt/msg00628.txt.bz2 On Tue, Sep 18, 2018, 6:56 AM Nick Clifton wrote: > 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. > Is there a way to avoid these instructions with GCC? As a general rule, for RTEMS we assume the code is properly generated for the target CPU and don't emulate missing instructions. Also for these cases, is there a flag implicitly set by GCC based on CPU specific flags to let assembly code.know not to use them. And are there mulilibs which do and do not use them? In the deeply embedded space, we assume the tools and user can generate code which doesn't need emulation traps. There is some responsibility on the toolchain to.make it possible. Thanks. --joel > > Cheers > Nick > > >