Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
> On Aug 30, 2018, at 9:02 PM, Jeff Law wrote: > > On 08/30/2018 10:58 AM, Richard Henderson wrote: >> On 08/28/2018 07:13 AM, Jeff Law wrote: >>> Please consider using function descriptors rather than trampolines. >>> This allows you to make the stack non-executable at all times which is >>> good from a security standpoint. The downside is the indirect calling >>> mechanism has to change slightly to distinguish between a simple >>> indirect call and one through a function descriptor (usually by having a >>> low bit on to indicate the latter). GIven this is an ABI change, now is >>> probably the last opportunity to make this change. >> >> Correct me if I'm wrong here: >> >> Define TARGET_CUSTOM_FUNCTION_DESCRIPTORS to an appropriate value -- easy >> for a >> RISC target -- and that's it. >> >> Further, it pretty much only gets used by the Ada front end. One should not >> expect these to be used by the C front end nested functions. > I thought it was used more extensively than that... Thanks for checking > into it though. > > Jeff My impression from reading the internals manual is that it's an alternative to trampolines -- and in fact it appears to suggest it's a superior alternative. I've been planning to try turning it on for pdp11 where executable stacks can be problematic. (For that matter, they are on lots of other machines -- which is why descriptors instead of trampolines sounds like a good thing.) paul
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/30/2018 10:58 AM, Richard Henderson wrote: > On 08/28/2018 07:13 AM, Jeff Law wrote: >> Please consider using function descriptors rather than trampolines. >> This allows you to make the stack non-executable at all times which is >> good from a security standpoint. The downside is the indirect calling >> mechanism has to change slightly to distinguish between a simple >> indirect call and one through a function descriptor (usually by having a >> low bit on to indicate the latter). GIven this is an ABI change, now is >> probably the last opportunity to make this change. > > Correct me if I'm wrong here: > > Define TARGET_CUSTOM_FUNCTION_DESCRIPTORS to an appropriate value -- easy for > a > RISC target -- and that's it. > > Further, it pretty much only gets used by the Ada front end. One should not > expect these to be used by the C front end nested functions. I thought it was used more extensively than that... Thanks for checking into it though. Jeff
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/28/2018 07:13 AM, Jeff Law wrote: > Please consider using function descriptors rather than trampolines. > This allows you to make the stack non-executable at all times which is > good from a security standpoint. The downside is the indirect calling > mechanism has to change slightly to distinguish between a simple > indirect call and one through a function descriptor (usually by having a > low bit on to indicate the latter). GIven this is an ABI change, now is > probably the last opportunity to make this change. Correct me if I'm wrong here: Define TARGET_CUSTOM_FUNCTION_DESCRIPTORS to an appropriate value -- easy for a RISC target -- and that's it. Further, it pretty much only gets used by the Ada front end. One should not expect these to be used by the C front end nested functions. r~
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On Thu, 30 Aug 2018, Stafford Horne wrote: > On Wed, Aug 29, 2018 at 04:38:45PM -0600, Jeff Law wrote: > > ps. Any plans for glibc? How complete is the qemu support? I'm having > > reasonable success building little chroot filesystems, then using qemu > > to do bootstrap testing within those chroots. > > Hello Juff, > > I was planning to start to look into glibc. We have a port here: > > https://github.com/openrisc/or1k-glibc > > I believe the contributor's all have FSF copyright signoff in place. However, > the port could not be upstreamed a few years ago due to the GCC port not being > able to go upstream. > > Once GCC is resolved I think it shouldn't be too much effort to get glibc back > in shape. You'll need to update it for all global changes in glibc since whenever work on that port started / whenever you last had it up to date with such cross-port changes in glibc. Also, note it's expected for new ports to have a shlib-versions file setting the minimum symbol version to that of the upstream glibc version the port actually goes in (and as already noted that file should also set an ABI-specific dynamic linker name, so for both reasons you have an ABI break from the previous glibc version). Look at review comments on other ports posted in the past year or two (RISC-V, ARC, NDS32, C-SKY) for an idea of various issues to watch out for and expectations for new ports and new port submissions (which include the upstream tools and Linux kernel port having everything required for building the glibc port with good test results). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On Wed, Aug 29, 2018 at 04:38:45PM -0600, Jeff Law wrote: > ps. Any plans for glibc? How complete is the qemu support? I'm having > reasonable success building little chroot filesystems, then using qemu > to do bootstrap testing within those chroots. Hello Juff, I was planning to start to look into glibc. We have a port here: https://github.com/openrisc/or1k-glibc I believe the contributor's all have FSF copyright signoff in place. However, the port could not be upstreamed a few years ago due to the GCC port not being able to go upstream. Once GCC is resolved I think it shouldn't be too much effort to get glibc back in shape. As Richard mentioned, qemu and musl is what we have been primarily testing on. Also, upstream support for newlib and uclibc-ng are in good shape. -Stafford
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/29/2018 03:38 PM, Jeff Law wrote: > As far as I'm concerned, you can commit this once the barriers & > speculation bits are fixed. If you can avoid trampolines, that can be a > follow-up. And long branches can definitely be punted until it's really > needed. Thanks. > ps. Any plans for glibc? How complete is the qemu support? I'm having > reasonable success building little chroot filesystems, then using qemu > to do bootstrap testing within those chroots. I think there are some plans for glibc. So far we've been using musl. QEMU support is complete and is what I've used so far for testing. r~
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On Wed, Aug 29, 2018 at 02:29:45PM -0700, Richard Henderson wrote: > On 08/28/2018 07:13 AM, Jeff Law wrote: > > I didn't see any provision for long vs short branches. How are branches > > to targets within the same function, but which are out of the range that > > can be encoded in a single instruction work? Similarly how is this > > handled for function calls? Note handling this gets even more complex > > with delay slots :( > > 26 bits, or +- 128MB, for all branches/calls. Considering most of the targets I have run on have 32mb ram I haven't thought about this. Thanks Richard for the replies. I will be working on the comments, documentation, deprecated macro updates and other trivial things. Once that is done Ill see if you have make any progress on the long jumps, function descriptors or other bigger changes and we can go from there with the plan for V2. -Stafford
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/29/2018 04:38 PM, Jeff Law wrote: > On 08/29/2018 03:29 PM, Richard Henderson wrote: > > As far as I'm concerned, you can commit this once the barriers & > speculation bits are fixed. If you can avoid trampolines, that can be a > follow-up. And long branches can definitely be punted until it's really > needed. Whoops. Those function comments that I pointed out earlier need updating too. Forgot about that part of my reply. jeff
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/29/2018 03:29 PM, Richard Henderson wrote: > On 08/28/2018 07:13 AM, Jeff Law wrote: >> Your port defines instruction scheduling, so please check that you're >> emitting the proper barriers, particularly in your epilogue code. In >> particular most ports need a barrier to prevent movement of register >> restores past the stack adjustment when the frame pointer is in use. > > Oops, yes we had that in the "old" compiler, but > didn't replicate the barriers in the rewrite. > > It's one of those "we didn't see it fail so we didn't fix it" > kind of things. Yea. But we both know the pain of trying to track this down when it finally exposes itself :-) > >> Please consider using function descriptors rather than trampolines. >> This allows you to make the stack non-executable at all times which is >> good from a security standpoint. The downside is the indirect calling >> mechanism has to change slightly to distinguish between a simple >> indirect call and one through a function descriptor (usually by having a >> low bit on to indicate the latter). GIven this is an ABI change, now is >> probably the last opportunity to make this change. > > We'll look into it. Thanks. I realize it may be too late, so I don't think it's a hard requirement. But it's worth looking into. > >> I didn't see any provision for long vs short branches. How are branches >> to targets within the same function, but which are out of the range that >> can be encoded in a single instruction work? Similarly how is this >> handled for function calls? Note handling this gets even more complex >> with delay slots :( > > 26 bits, or +- 128MB, for all branches/calls. > > You'd have to have a *really* big executable to overflow this. > I'd expect to fix out-of-range calls, if necessary, in the linker > with some sort of branch trampoline. More bits than expected :-) I don't mind punting this until you have to resolve it due to big executables. > >> I don't know if openrisc has any speculation capabilities. Please >> define the SPECULATION_SAFE_VALUE hook appropriately for your >> architecture. If you don't have speculation, define it to >> speculation_safe_value_not_needed. > > None of the implementations do speculation. THen I think you can just define it to speculation_safe_value_not_needed. As far as I'm concerned, you can commit this once the barriers & speculation bits are fixed. If you can avoid trampolines, that can be a follow-up. And long branches can definitely be punted until it's really needed. Jeff ps. Any plans for glibc? How complete is the qemu support? I'm having reasonable success building little chroot filesystems, then using qemu to do bootstrap testing within those chroots.
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On Tue, Aug 28, 2018 at 08:13:48AM -0600, Jeff Law wrote: > On 08/26/2018 03:18 PM, Stafford Horne wrote: > > -mm-dd Stafford Horne > > Richard Henderson > > > > gcc/ChangeLog: > > > > * common/config/or1k/or1k-common.c: New file. > > * config/or1k/*: New. > > * config.gcc (or1k*-*-*): New. > > * configure.ac (or1k*-*-*): New test for openrisc tls. > > * configure: Regenerated. > So I still want to do another looksie, but a few comments beyond what > Joseph has already pointed out. > > Many functions are documented with "WORKER for xyz." kinds of comments. > Policy is each function should have a comment which describes what the > function does at a high level as well as the inputs/outputs. I will work on cleaning up the comments. I am hoping the OpenRISC port would be a easy to understand port for RISC processes for others getting started. Fixing these comments will really help with that. The other points Richard has expressed on his reply. Ill also add some commentary there. Thank you for the review. -Stafford
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/28/2018 07:13 AM, Jeff Law wrote: > Your port defines instruction scheduling, so please check that you're > emitting the proper barriers, particularly in your epilogue code. In > particular most ports need a barrier to prevent movement of register > restores past the stack adjustment when the frame pointer is in use. Oops, yes we had that in the "old" compiler, but didn't replicate the barriers in the rewrite. It's one of those "we didn't see it fail so we didn't fix it" kind of things. > Please consider using function descriptors rather than trampolines. > This allows you to make the stack non-executable at all times which is > good from a security standpoint. The downside is the indirect calling > mechanism has to change slightly to distinguish between a simple > indirect call and one through a function descriptor (usually by having a > low bit on to indicate the latter). GIven this is an ABI change, now is > probably the last opportunity to make this change. We'll look into it. > I didn't see any provision for long vs short branches. How are branches > to targets within the same function, but which are out of the range that > can be encoded in a single instruction work? Similarly how is this > handled for function calls? Note handling this gets even more complex > with delay slots :( 26 bits, or +- 128MB, for all branches/calls. You'd have to have a *really* big executable to overflow this. I'd expect to fix out-of-range calls, if necessary, in the linker with some sort of branch trampoline. > I don't know if openrisc has any speculation capabilities. Please > define the SPECULATION_SAFE_VALUE hook appropriately for your > architecture. If you don't have speculation, define it to > speculation_safe_value_not_needed. None of the implementations do speculation. r~
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On 08/26/2018 03:18 PM, Stafford Horne wrote: > -mm-dd Stafford Horne > Richard Henderson > > gcc/ChangeLog: > > * common/config/or1k/or1k-common.c: New file. > * config/or1k/*: New. > * config.gcc (or1k*-*-*): New. > * configure.ac (or1k*-*-*): New test for openrisc tls. > * configure: Regenerated. So I still want to do another looksie, but a few comments beyond what Joseph has already pointed out. Many functions are documented with "WORKER for xyz." kinds of comments. Policy is each function should have a comment which describes what the function does at a high level as well as the inputs/outputs. Your port defines instruction scheduling, so please check that you're emitting the proper barriers, particularly in your epilogue code. In particular most ports need a barrier to prevent movement of register restores past the stack adjustment when the frame pointer is in use. Please consider using function descriptors rather than trampolines. This allows you to make the stack non-executable at all times which is good from a security standpoint. The downside is the indirect calling mechanism has to change slightly to distinguish between a simple indirect call and one through a function descriptor (usually by having a low bit on to indicate the latter). GIven this is an ABI change, now is probably the last opportunity to make this change. I didn't see any provision for long vs short branches. How are branches to targets within the same function, but which are out of the range that can be encoded in a single instruction work? Similarly how is this handled for function calls? Note handling this gets even more complex with delay slots :( I don't know if openrisc has any speculation capabilities. Please define the SPECULATION_SAFE_VALUE hook appropriately for your architecture. If you don't have speculation, define it to speculation_safe_value_not_needed. I don't see anything glaringly wrong, but would like to take another looksie once the issues raised by Joseph and the function doc issue are addressed. Jeff ps. 1/3 and 2/3 look fine.
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
Hi Joseph, On Mon, Aug 27, 2018 at 05:24:51PM +, Joseph Myers wrote: > On Mon, 27 Aug 2018, Stafford Horne wrote: > > > gcc/config/or1k/elf.opt | 33 + > > > gcc/config/or1k/or1k.opt | 41 + > > Command-line options need documenting in invoke.texi. This patch is > missing documentation updates. Please see sourcebuild.texi, section "Back > End", for a list of the various places that need updating for a new target > architecture, including the various places in the documentation that need > updating if the architecture has any instances of the feature documented > there. Sure I will document these. I was thinking about what needed to be documented, thanks for pointing this out. > > +#define TARGET_FLOAT_FORMAT IEEE_FLOAT_FORMAT > > This target macro was obsoleted in 2008. You shouldn't be defining it > here. > > Whereas TARGET_FLOAT_FORMAT is missing a poisoning in system.h, such as > should be done for obsolete target macros to make it easy to spot when an > out-of-tree port is defining them, the obsolete NO_IMPLICIT_EXTERN_C, > which you're defining in or1k/linux.h, is properly poisoned in system.h, > so I'd have expected the build for that target to fail. I will fix the TARGET_FLOAT_FORMAT. However, I am not sure why NO_IMPLICIT_EXTERN_C is not causing a compile failure. This definitely has been working and tested with musl libc. I will let you know what I find. > > You're using /lib/ld.so.1 as GLIBC_DYNAMIC_LINKER. Preferred glibc > practice for new ports is to have an architecture-specific, ABI-specific > dynamic linker name, not shared with any other ABI or architecture, to > support distributions using multi-arch directory arrangements. Sure, I will do that. We have an existing glibc port which I haven't touched before, I will see what it expects and set it up that way. > Why the __BIG_ENDIAN__ built-in macro? Since we have the > architecture-independent __BYTE_ORDER__ macro, new architectures shouldn't > generally define their own macros related to byte-order (especially for an > architecture that only supports one endianness!) unless it's trying to > implement some compiler-independent API for that architecture that says > such macros are defined. I think this just comes from me copying an example from m32r. I don't think its needed and I will remove it. Thank you for the review, I would not have spotted these. If anything is missing in the gccint docs which might make it easier for future porters to not make these mistakes I will try to add it too. -Stafford
Re: [PATCH 3/3] or1k: gcc: initial support for openrisc
On Mon, 27 Aug 2018, Stafford Horne wrote: > gcc/config/or1k/elf.opt | 33 + > gcc/config/or1k/or1k.opt | 41 + Command-line options need documenting in invoke.texi. This patch is missing documentation updates. Please see sourcebuild.texi, section "Back End", for a list of the various places that need updating for a new target architecture, including the various places in the documentation that need updating if the architecture has any instances of the feature documented there. > +#define TARGET_FLOAT_FORMAT IEEE_FLOAT_FORMAT This target macro was obsoleted in 2008. You shouldn't be defining it here. Whereas TARGET_FLOAT_FORMAT is missing a poisoning in system.h, such as should be done for obsolete target macros to make it easy to spot when an out-of-tree port is defining them, the obsolete NO_IMPLICIT_EXTERN_C, which you're defining in or1k/linux.h, is properly poisoned in system.h, so I'd have expected the build for that target to fail. You're using /lib/ld.so.1 as GLIBC_DYNAMIC_LINKER. Preferred glibc practice for new ports is to have an architecture-specific, ABI-specific dynamic linker name, not shared with any other ABI or architecture, to support distributions using multi-arch directory arrangements. Why the __BIG_ENDIAN__ built-in macro? Since we have the architecture-independent __BYTE_ORDER__ macro, new architectures shouldn't generally define their own macros related to byte-order (especially for an architecture that only supports one endianness!) unless it's trying to implement some compiler-independent API for that architecture that says such macros are defined. -- Joseph S. Myers jos...@codesourcery.com
