| Message ID | 53649694.6050508@marino.st |
|---|---|
| State | New |
| Headers | show |
On 3 May 2014 08:11, John Marino wrote: > On 5/2/2014 22:15, Joseph S. Myers wrote: >> On Fri, 2 May 2014, John Marino wrote: >> >>> 1) I don't know which type definitions are missing (iow, the important >>> ones from sys/type.h that are required to build gcc) >> >> The default presumption should be: >> >> * <stddef.h> from GCC provides what it needs to provide; nothing extra is >> needed and such a #include should not be needed at all. >> >> * Special measures to avoid duplicate typedefs (where some other header >> also defines one of the typedefs defined in <stddef.h>) aren't in fact >> needed, because GCC allows duplicate typedefs in system headers (even >> outside C11 mode - in C11 mode it's a standard feature). >> >> So try removing that #include. If that causes problems, investigate based >> on the actual problems seen. > > Hi Joseph, > Removing the include worked after also removing the #ifdef __DragonFly > with regards to the rune_t type definition. > > I built gcc with a full bootstraps on both DragonFly platforms > successfully. stddef.h is much simpler now: > > --- gcc/ginclude/stddef.h.orig > +++ gcc/ginclude/stddef.h > @@ -133,6 +133,7 @@ > #ifndef _BSD_PTRDIFF_T_ > #ifndef ___int_ptrdiff_t_h > #ifndef _GCC_PTRDIFF_T > +#ifndef _PTRDIFF_T_DECLARED /* DragonFly */ > #define _PTRDIFF_T > #define _T_PTRDIFF_ > #define _T_PTRDIFF > @@ -141,10 +142,12 @@ > #define _BSD_PTRDIFF_T_ > #define ___int_ptrdiff_t_h > #define _GCC_PTRDIFF_T > +#define _PTRDIFF_T_DECLARED > #ifndef __PTRDIFF_TYPE__ > #define __PTRDIFF_TYPE__ long int > #endif > typedef __PTRDIFF_TYPE__ ptrdiff_t; > +#endif /* _PTRDIFF_T_DECLARED */ > #endif /* _GCC_PTRDIFF_T */ > #endif /* ___int_ptrdiff_t_h */ > #endif /* _BSD_PTRDIFF_T_ */ > @@ -198,6 +201,7 @@ > #define _GCC_SIZE_T > #define _SIZET_ > #if (defined (__FreeBSD__) && (__FreeBSD__ >= 5)) \ > + || defined(__DragonFly__) \ > || defined(__FreeBSD_kernel__) > /* __size_t is a typedef on FreeBSD 5, must not trash it. */ > #elif defined (__VMS__) > > > revised patchset : > http://leaf.dragonflybsd.org/~marino/gcc-df-target/patches/patch-dragonfly-target > revised changelog : > http://leaf.dragonflybsd.org/~marino/gcc-df-target/changelog_entries/gcc_ChangeLog_entry.txt > revised commit msg: > http://leaf.dragonflybsd.org/~marino/gcc-df-target/proposed_commit-msg.txt > > Good catch! Does the rest of the patch set look good to you? I think > all the non-obvious patches have been reviewed collectively by various > people now and may be ready to be approved now. Ian's approved the libiberty.h change, Joseph's approved the stddef.h change, I've approved the libstdc++ parts. IIUC it still needs explicit approval for the rest, e.g. trivial adjustments to configuration stuff in libitm and libcilkrts. Are there specific maintainers for those libs? The rest look obvious to me, it doesn't touch other targets at all except for one bit that replaces a check for __FreeBSD__ >= 7 with a grep for the dl_iterate_phdr function in a system header, which only affects FreeBSD and looks OK to me. Anyone willing to give it an overall approval?
On 05/08/14 07:14, Jonathan Wakely wrote: > > Ian's approved the libiberty.h change, Joseph's approved the stddef.h > change, I've approved the libstdc++ parts. > > IIUC it still needs explicit approval for the rest, e.g. trivial > adjustments to configuration stuff in libitm and libcilkrts. Are there > specific maintainers for those libs? > > The rest look obvious to me, it doesn't touch other targets at all > except for one bit that replaces a check for __FreeBSD__ >= 7 with a > grep for the dl_iterate_phdr function in a system header, which only > affects FreeBSD and looks OK to me. > > Anyone willing to give it an overall approval? I'll take a look at the rest. I mostly wanted someone else to deal with stddef.h :-) jeff
On 5/8/2014 15:32, Jeff Law wrote: > On 05/08/14 07:14, Jonathan Wakely wrote: >> >> Anyone willing to give it an overall approval? > I'll take a look at the rest. I mostly wanted someone else to deal with > stddef.h :-) Thanks Jeff! I'm am very appreciative of that. John
On 05/03/14 01:11, John Marino wrote: > > > revised patchset : > http://leaf.dragonflybsd.org/~marino/gcc-df-target/patches/patch-dragonfly-target > revised changelog : > http://leaf.dragonflybsd.org/~marino/gcc-df-target/changelog_entries/gcc_ChangeLog_entry.txt > revised commit msg: > http://leaf.dragonflybsd.org/~marino/gcc-df-target/proposed_commit-msg.txt > > Good catch! Does the rest of the patch set look good to you? I think > all the non-obvious patches have been reviewed collectively by various > people now and may be ready to be approved now. In config.gcc: + no | gnat | single) + # Let these non-posix thread selections fall through if requested Support for "gnat" as a thread model was removed in 2011. So I think you need to remove that case. configure.ac: + *-*-dragonfly* | *-*-freebsd*) + if grep dl_iterate_phdr $target_header_dir/sys/link_elf.h > /dev/null 2>&1; then + gcc_cv_target_dl_iterate_phdr=yes + else + gcc_cv_target_dl_iterate_phdr=no + fi + ;; Presumably you intended to change freebsd* here. Just want a confirmation. I haven't worked on the *bsd platforms in about 20 years, so I have no idea if this is right for them in general. I see you have a dragonfly-stdint.h. Is there a particular reason why you can't use the freebsd-stdint.h? I didn't check every type, but a quick glance makes me think they ought to be equivalent. Similarly for dragonfly.opt. It looks like there's a fair amount of duplication in config/dragonfly.h and config/i386/dragonfly but I don't see an easy way to fix that. So, I'll let that go. I'm going to trust the unwind code works and isn't duplicating something from somewhere else that ought to instead be shared. So it basically looks good. Can you fix the config.gcc nit and determine if we can (and should) share files with freebsd. Repost after those fixes and we should be ready to go. And one final thing, do you have a copyright assignment on file with the FSF? jeff
On 5/9/2014 07:26, Jeff Law wrote: > On 05/03/14 01:11, John Marino wrote: > > In config.gcc: > > + no | gnat | single) > + # Let these non-posix thread selections fall through if requested > Support for "gnat" as a thread model was removed in 2011. So I think > you need to remove that case. I realized that the gnat thread mechanism had been removed a couple of days ago, but I didn't want to invalidate the ongoing review since it was a not really an issue. I'll make the change now. This hunk was obviously created when it did exist. > configure.ac: > > + *-*-dragonfly* | *-*-freebsd*) > + if grep dl_iterate_phdr $target_header_dir/sys/link_elf.h > > /dev/null 2>&1; then > + gcc_cv_target_dl_iterate_phdr=yes > + else > + gcc_cv_target_dl_iterate_phdr=no > + fi > + ;; > Presumably you intended to change freebsd* here. Just want a > confirmation. I haven't worked on the *bsd platforms in about 20 years, > so I have no idea if this is right for them in general. Yes, this is intentional. This is why I also did a full testsuite on FreeBSD as well as DragonFly to prove that still worked. NetBSD and OpenBSD would be handled similarly when the time comes, but they would need to grep a different header. > I see you have a dragonfly-stdint.h. Is there a particular reason why > you can't use the freebsd-stdint.h? I didn't check every type, but a > quick glance makes me think they ought to be equivalent. > > Similarly for dragonfly.opt. And there is already precedent for each system to have its own files: freebsd.opt freebsd-stdint.h openbsd.opt openbsd-stdint.h netbsd.opt ( The dragonfly-stdint.h is cleaner than freebsd-stdint.h as well. > It looks like there's a fair amount of duplication in config/dragonfly.h > and config/i386/dragonfly but I don't see an easy way to fix that. So, > I'll let that go. While similar due to heritage, and also due to a conscious effort to keep the userland compatible where a difference isn't specifically needed, DragonFly is not FreeBSD. We've had a challenge with software that consider them to be equivalent in every aspect. Sometimes changes are made against a FreeBSD file that is not valid for DragonFly, so even if they are equivalent today they may not be in the future. We prefer separate configuration files like NetBSD and OpenBSD have in general. by the way config/dragonfly.h and config/i386/dragonfly.h are significantly different that FreeBSD counterparts. And we eliminated the equivalent of config/i386/freebsd64.h by combining it's functionality into config/i386/dragonfly.h. There are also platform-specific differences there so there is no question that DragonFly needs its own header files. > I'm going to trust the unwind code works and isn't duplicating something > from somewhere else that ought to instead be shared. Not only is it not duplicated, FreeBSD needs its own, different version (FreeBSD is currently missing unwind functionality). I have the patch and that's a separate submission (out of scope for DragonFly target creation). Believe me, if there is one thing you would not want to duplicate, it's MD support code. FYI NetBSD and OpenBSD are missing this functionality too. > So it basically looks good. Can you fix the config.gcc nit and > determine if we can (and should) share files with freebsd. Repost after > those fixes and we should be ready to go. 1) Patch updated online as requested 2) At this exact point in time, we probably can share the files 3) I might debate that we should share the files - that would imply reviewing the existing counterpart files for NetBSD and OpenBSD and combining when equivalent. > And one final thing, do you have a copyright assignment on file with the > FSF? Yes, since 2011. I mentioned that in the very first post of the thread along with the associated assignment number. > jeff Thank you very much for the overarching review Jeff! John
On 05/09/14 01:14, John Marino wrote: > On 5/9/2014 07:26, Jeff Law wrote: >> On 05/03/14 01:11, John Marino wrote: >> >> In config.gcc: >> >> + no | gnat | single) >> + # Let these non-posix thread selections fall through if requested >> Support for "gnat" as a thread model was removed in 2011. So I think >> you need to remove that case. > > I realized that the gnat thread mechanism had been removed a couple of > days ago, but I didn't want to invalidate the ongoing review since it > was a not really an issue. I'll make the change now. This hunk was > obviously created when it did exist. No problem. > >> configure.ac: >> >> + *-*-dragonfly* | *-*-freebsd*) >> + if grep dl_iterate_phdr $target_header_dir/sys/link_elf.h > >> /dev/null 2>&1; then >> + gcc_cv_target_dl_iterate_phdr=yes >> + else >> + gcc_cv_target_dl_iterate_phdr=no >> + fi >> + ;; >> Presumably you intended to change freebsd* here. Just want a >> confirmation. I haven't worked on the *bsd platforms in about 20 years, >> so I have no idea if this is right for them in general. > > > Yes, this is intentional. This is why I also did a full testsuite on > FreeBSD as well as DragonFly to prove that still worked. OK. Just wanted to be sure. As I mentioned, it's been a *long* time since I did anything with the *bsd ports. > > NetBSD and OpenBSD would be handled similarly when the time comes, but > they would need to grep a different header. > > >> I see you have a dragonfly-stdint.h. Is there a particular reason why >> you can't use the freebsd-stdint.h? I didn't check every type, but a >> quick glance makes me think they ought to be equivalent. >> >> Similarly for dragonfly.opt. > > And there is already precedent for each system to have its own files: > > freebsd.opt freebsd-stdint.h > openbsd.opt openbsd-stdint.h > netbsd.opt ( > > The dragonfly-stdint.h is cleaner than freebsd-stdint.h as well. Yea, there's always a bit of a natural tension between having this kind of stuff duplicated vs sharing when their contents are common. I lean towards the latter in this case for a variety of reasons. > > While similar due to heritage, and also due to a conscious effort to > keep the userland compatible where a difference isn't specifically > needed, DragonFly is not FreeBSD. We've had a challenge with software > that consider them to be equivalent in every aspect. I certainly understand having done similar stuff in the past. > > Sometimes changes are made against a FreeBSD file that is not valid for > DragonFly, so even if they are equivalent today they may not be in the > future. We prefer separate configuration files like NetBSD and OpenBSD > have in general. Right and this is the most important counter-argument to sharing. They're compatible today, but will they be tomorrow? It sounds like Dragonfly has a bit of a mandate to be different than FreeBSD, so there's probably more than the usual chance this stuff could diverge in the future. > > by the way config/dragonfly.h and config/i386/dragonfly.h are > significantly different that FreeBSD counterparts. And we eliminated > the equivalent of config/i386/freebsd64.h by combining it's > functionality into config/i386/dragonfly.h. There are also > platform-specific differences there so there is no question that > DragonFly needs its own header files. I saw that when scanning dragfonfly.h and freebsd.h to see how much commonality there was between them. > >> I'm going to trust the unwind code works and isn't duplicating something >> from somewhere else that ought to instead be shared. > > Not only is it not duplicated, FreeBSD needs its own, different version > (FreeBSD is currently missing unwind functionality). I have the patch > and that's a separate submission (out of scope for DragonFly target > creation). Believe me, if there is one thing you would not want to > duplicate, it's MD support code. FYI NetBSD and OpenBSD are missing > this functionality too. > > >> So it basically looks good. Can you fix the config.gcc nit and >> determine if we can (and should) share files with freebsd. Repost after >> those fixes and we should be ready to go. > > 1) Patch updated online as requested > 2) At this exact point in time, we probably can share the files > 3) I might debate that we should share the files - that would imply > reviewing the existing counterpart files for NetBSD and OpenBSD and > combining when equivalent. Let's go ahead and keep the files separate. We can always go back and combine them at a later date if we see the need. So with that in mind, the patch is good to go with the gnat thread stuff removed. Do you have write access, or do you you need someone to commit the change for you? Jeff
On 5/12/2014 18:59, Jeff Law wrote: > On 05/09/14 01:14, John Marino wrote: >> >> 1) Patch updated online as requested >> 2) At this exact point in time, we probably can share the files >> 3) I might debate that we should share the files - that would imply >> reviewing the existing counterpart files for NetBSD and OpenBSD and >> combining when equivalent. > Let's go ahead and keep the files separate. We can always go back and > combine them at a later date if we see the need. > > So with that in mind, the patch is good to go with the gnat thread stuff > removed. > > Do you have write access, or do you you need someone to commit the > change for you? Thanks, Jeff! I do not have write access, but jwakely has graciously offered to commit this patchset when it achieves approval, so I guess he's on deck! John
On 05/12/14 11:10, John Marino wrote: > On 5/12/2014 18:59, Jeff Law wrote: >> On 05/09/14 01:14, John Marino wrote: >>> >>> 1) Patch updated online as requested >>> 2) At this exact point in time, we probably can share the files >>> 3) I might debate that we should share the files - that would imply >>> reviewing the existing counterpart files for NetBSD and OpenBSD and >>> combining when equivalent. >> Let's go ahead and keep the files separate. We can always go back and >> combine them at a later date if we see the need. >> >> So with that in mind, the patch is good to go with the gnat thread stuff >> removed. >> >> Do you have write access, or do you you need someone to commit the >> change for you? > > Thanks, Jeff! > I do not have write access, but jwakely has graciously offered to commit > this patchset when it achieves approval, so I guess he's on deck! OK. It's Jon's :-) Jeff
On 12 May 2014 18:14, Jeff Law wrote: > On 05/12/14 11:10, John Marino wrote: >> >> On 5/12/2014 18:59, Jeff Law wrote: >>> >>> On 05/09/14 01:14, John Marino wrote: >>>> >>>> >>>> 1) Patch updated online as requested >>>> 2) At this exact point in time, we probably can share the files >>>> 3) I might debate that we should share the files - that would imply >>>> reviewing the existing counterpart files for NetBSD and OpenBSD and >>>> combining when equivalent. >>> >>> Let's go ahead and keep the files separate. We can always go back and >>> combine them at a later date if we see the need. >>> >>> So with that in mind, the patch is good to go with the gnat thread stuff >>> removed. >>> >>> Do you have write access, or do you you need someone to commit the >>> change for you? >> >> >> Thanks, Jeff! >> I do not have write access, but jwakely has graciously offered to commit >> this patchset when it achieves approval, so I guess he's on deck! > > OK. It's Jon's :-) Great, I'll coordinate with John and aim to get it committed before Friday.
--- gcc/ginclude/stddef.h.orig +++ gcc/ginclude/stddef.h @@ -133,6 +133,7 @@ #ifndef _BSD_PTRDIFF_T_ #ifndef ___int_ptrdiff_t_h #ifndef _GCC_PTRDIFF_T +#ifndef _PTRDIFF_T_DECLARED /* DragonFly */ #define _PTRDIFF_T #define _T_PTRDIFF_ #define _T_PTRDIFF @@ -141,10 +142,12 @@ #define _BSD_PTRDIFF_T_ #define ___int_ptrdiff_t_h #define _GCC_PTRDIFF_T +#define _PTRDIFF_T_DECLARED #ifndef __PTRDIFF_TYPE__ #define __PTRDIFF_TYPE__ long int #endif typedef __PTRDIFF_TYPE__ ptrdiff_t; +#endif /* _PTRDIFF_T_DECLARED */ #endif /* _GCC_PTRDIFF_T */ #endif /* ___int_ptrdiff_t_h */ #endif /* _BSD_PTRDIFF_T_ */ @@ -198,6 +201,7 @@ #define _GCC_SIZE_T #define _SIZET_ #if (defined (__FreeBSD__) && (__FreeBSD__ >= 5)) \ + || defined(__DragonFly__) \ || defined(__FreeBSD_kernel__) /* __size_t is a typedef on FreeBSD 5, must not trash it. */ #elif defined (__VMS__)