* [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE @ 2018-06-27 14:27 Rasmus Villemoes 2018-09-03 12:11 ` Olivier Hainque 2018-10-08 13:14 ` [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper Rasmus Villemoes 0 siblings, 2 replies; 9+ messages in thread From: Rasmus Villemoes @ 2018-06-27 14:27 UTC (permalink / raw) To: gcc-patches; +Cc: bkorb, Olivier Hainque, Rasmus Villemoes VxWorks' regs.h does include some files that need types defined in vxTypesOld.h, and it does not itself include that header, directly or indirectly. Moreover, vxTypesOld.h ends up pulling in definitions of various cpufamily macros (from types/vxCpu.h) that are also needed directly by regs.h. However, when compiling some assembly files that #include "regs.h", the typedefs in vxTypesOld.h break the build: /tmp/ccPxG4gA.s: Assembler messages: /tmp/ccPxG4gA.s:1: Error: unrecognized opcode: `typedef' /tmp/ccPxG4gA.s:2: Error: unrecognized opcode: `typedef' etc. The simplest fix is to guard the include of vxTypesOld.h by !defined(_ASMLANGUAGE). This should not affect C code, and existing assembly files that include regs.h must already have arranged for including types/vxCpu.h prior to including regs.h. ==changelog== fixincludes/ * inclhack.def (AAB_vxworks_regs_vxtypes): Guard include of types/vxTypesOld.h by #ifndef _ASMLANGUAGE. * fixincl.x: Regenerate. --- fixincludes/inclhack.def | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def index c1f5a13eda4..bac0079b69f 100644 --- a/fixincludes/inclhack.def +++ b/fixincludes/inclhack.def @@ -426,7 +426,9 @@ fix = { replace = <<- _EndOfHeader_ #ifndef _REGS_H #define _REGS_H + #ifndef _ASMLANGUAGE #include <types/vxTypesOld.h> + #endif #include_next <arch/../regs.h> #endif _EndOfHeader_; -- 2.16.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-06-27 14:27 [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE Rasmus Villemoes @ 2018-09-03 12:11 ` Olivier Hainque 2018-09-03 13:20 ` Rasmus Villemoes 2018-10-08 13:14 ` [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper Rasmus Villemoes 1 sibling, 1 reply; 9+ messages in thread From: Olivier Hainque @ 2018-09-03 12:11 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Olivier Hainque, gcc-patches, bkorb Hi Rasmus, > On 27 Jun 2018, at 16:27, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: > * inclhack.def (AAB_vxworks_regs_vxtypes): Guard include of > types/vxTypesOld.h by #ifndef _ASMLANGUAGE. > * fixincl.x: Regenerate. > --- > fixincludes/inclhack.def | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def > index c1f5a13eda4..bac0079b69f 100644 > --- a/fixincludes/inclhack.def > +++ b/fixincludes/inclhack.def > @@ -426,7 +426,9 @@ fix = { > replace = <<- _EndOfHeader_ > #ifndef _REGS_H > #define _REGS_H > + #ifndef _ASMLANGUAGE > #include <types/vxTypesOld.h> > + #endif > #include_next <arch/../regs.h> > #endif > _EndOfHeader_; This will really look puzzling to a reader and would at least deserve a comment. How do we not get in assembly the problems we'd get in C when not including vxTypesOld ? Olivier ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-09-03 12:11 ` Olivier Hainque @ 2018-09-03 13:20 ` Rasmus Villemoes 2018-09-05 9:38 ` Olivier Hainque 0 siblings, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2018-09-03 13:20 UTC (permalink / raw) To: Olivier Hainque; +Cc: gcc-patches, bkorb On 2018-09-03 14:11, Olivier Hainque wrote: > Hi Rasmus, > >> On 27 Jun 2018, at 16:27, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: >> * inclhack.def (AAB_vxworks_regs_vxtypes): Guard include of >> types/vxTypesOld.h by #ifndef _ASMLANGUAGE. >> * fixincl.x: Regenerate. >> --- >> fixincludes/inclhack.def | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def >> index c1f5a13eda4..bac0079b69f 100644 >> --- a/fixincludes/inclhack.def >> +++ b/fixincludes/inclhack.def >> @@ -426,7 +426,9 @@ fix = { >> replace = <<- _EndOfHeader_ >> #ifndef _REGS_H >> #define _REGS_H >> + #ifndef _ASMLANGUAGE >> #include <types/vxTypesOld.h> >> + #endif >> #include_next <arch/../regs.h> >> #endif >> _EndOfHeader_; > > This will really look puzzling to a reader and would > at least deserve a comment. Hm, yes, that might be a good idea. > How do we not get in assembly the problems we'd get in C > when not including vxTypesOld ? Well, I don't know why types/vxTypesOld.h got shoehorned into the fixinclude regs.h [1]. A better fix would be to remove that #include completely, making the fixinclude regs.h a pure wrapper for vxworks regs.h. However, I was afraid that might break existing C code that has come to rely on "#include <regs.h>" pulling in types/vxTypesOld.h (or any of the headers included from that), and it's not really possible to know if such code exists - other than trying it and waiting for emails to arrive.(*) As I wrote, including the vxworks regs.h (from assembly or C) requires that types/vxCpu.h is already included, because it contains directives such as #if CPU_FAMILY==I960 ... #if CPU_FAMILY==MC680X0 Hence my "fix" relies on the fact that any existing assembly source that includes regs.h must already have arranged for types/vxCpu.h to be included, one way or another. I could add an explicit include of that (maybe just in the _ASMLANGUAGE case), but that could easily cause problems of its own. Now, if one could rely on all existing C code having been through a non-gcc compiler, the same argument could be applied and my above worry (*) would be unfounded, but... [1] There's not much explanation in 74ee6ab5823c . Also, while since fixed, that commit added wrong definitions of UINT8_MAX et al (type promotion issue). So maybe that commit was just a bit hastily added. Rasmus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-09-03 13:20 ` Rasmus Villemoes @ 2018-09-05 9:38 ` Olivier Hainque 2018-09-13 0:46 ` Rasmus Villemoes 0 siblings, 1 reply; 9+ messages in thread From: Olivier Hainque @ 2018-09-05 9:38 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Olivier Hainque, gcc-patches, bkorb Hi Rasmus, > On 3 Sep 2018, at 15:20, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: >> How do we not get in assembly the problems we'd get in C >> when not including vxTypesOld ? Answering part of my own question: turns out that some pieces included via regs.h are already taking care of the _ASMLANGUAGE case. > Well, I don't know why types/vxTypesOld.h got shoehorned into the > fixinclude regs.h [1]. A better fix would be to remove that #include > completely, making the fixinclude regs.h a pure wrapper for vxworks > regs.h. However, I was afraid that might break existing C code that has > come to rely on "#include <regs.h>" pulling in types/vxTypesOld.h (or > any of the headers included from that), and it's not really possible to > know if such code exists - other than trying it and waiting for emails > to arrive.(*) > As I wrote, including the vxworks regs.h (from assembly or C) requires > that types/vxCpu.h is already included, because it contains directives > such as > > #if CPU_FAMILY==I960 > ... > #if CPU_FAMILY==MC680X0 > Hence my "fix" relies on the fact that any existing assembly source that > includes regs.h must already have arranged for types/vxCpu.h to be > included, one way or another. I could add an explicit include of that > (maybe just in the _ASMLANGUAGE case), but that could easily cause > problems of its own. Now, if one could rely on all existing C code > having been through a non-gcc compiler, the same argument could be > applied and my above worry (*) would be unfounded, but... I think we should either do a fixinclude that would "work" for C and ASM (like #include vxCpu for ASM, vxTypesOld otherwise), or simply remove this hack (just using the fixinclude parlance here). My inclination would be for the latter. First, I'm not convinced fixincludes should be in the business of dealing with that kind of issue, the proper resolution of which depends on context. Then if this triggers a failure for some user, it would only show up for people upgrading the toolchain, which always calls for particular attention and often goes with adjustments. The symptom would be a compilation failure, easy to address if you can modify sources, with changes that you'd need to do if you were compiling with the system toolchain anyway, and which can be done with a manual fixinclude like trick if really needed. Finally, - this would only be visible in cases where the headers needed by regs.h aren't already #included, - there are probably not many users of the upstream gcc for VxWorks, and - I know that at least some of them (us) don't use fixincludes to begin with. So the risk of breaking existing C code seems very low for starters. What happens on your end if you just remove the hack ? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-09-05 9:38 ` Olivier Hainque @ 2018-09-13 0:46 ` Rasmus Villemoes 2018-09-14 13:02 ` Olivier Hainque 0 siblings, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2018-09-13 0:46 UTC (permalink / raw) To: Olivier Hainque; +Cc: gcc-patches, bkorb On 2018-09-05 11:38, Olivier Hainque wrote: > Hi Rasmus, > > I think we should either do a fixinclude that would "work" for > C and ASM (like #include vxCpu for ASM, vxTypesOld otherwise), or > simply remove this hack (just using the fixinclude parlance here). > > My inclination would be for the latter. > [snip] > > What happens on your end if you just remove the hack ? > Unfortunately, the libstdc++ build breaks: In file included from /usr/powerpc-wrs-vxworks/wind_base/target/h/regs.h:66:0, from /bld/vestas/auto/work.20180912.214646/gcc-build/gcc/include-fixed/setjmp.h:57, from /bld/vestas/auto/work.20180912.214646/gcc-build/powerpc-wrs-vxworks/libstdc++-v3/include/csetjmp:42, from /bld/vestas/auto/work.20180912.214646/gcc-src/libstdc++-v3/include/precompiled/stdc++.h:42: /usr/powerpc-wrs-vxworks/wind_base/target/h/arch/ppc/regsPpc.h:33:5: error: 'UINT32' does not name a type UINT32 cr; /* condition register */ ^~~~~~ I'm happy to add an include of vxCpu in the _ASMLANGUAGE case, along with a big comment. But, it's also a small enough patch that we can carry it internally, if you prefer that we don't touch this hack upstream. Thanks, Rasmus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-09-13 0:46 ` Rasmus Villemoes @ 2018-09-14 13:02 ` Olivier Hainque 2018-10-08 11:59 ` Rasmus Villemoes 0 siblings, 1 reply; 9+ messages in thread From: Olivier Hainque @ 2018-09-14 13:02 UTC (permalink / raw) To: Rasmus Villemoes Cc: Olivier Hainque, GCC Patches, bkorb, Jérôme Lambourg > On 13 Sep 2018, at 00:25, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: >> What happens on your end if you just remove the hack ? > Unfortunately, the libstdc++ build breaks: > > In file included from > /usr/powerpc-wrs-vxworks/wind_base/target/h/regs.h:66:0, > from > /bld/vestas/auto/work.20180912.214646/gcc-build/gcc/include-fixed/setjmp.h:57, > from > /bld/vestas/auto/work.20180912.214646/gcc-build/powerpc-wrs-vxworks/libstdc++-v3/include/csetjmp:42, > from > /bld/vestas/auto/work.20180912.214646/gcc-src/libstdc++-v3/include/precompiled/stdc++.h:42: > /usr/powerpc-wrs-vxworks/wind_base/target/h/arch/ppc/regsPpc.h:33:5: > error: 'UINT32' does not name a type > UINT32 cr; /* condition register */ > ^~~~~~ Ah, I see. Thanks for the experiment. > I'm happy to add an include of vxCpu in the _ASMLANGUAGE case, along > with a big comment. But, it's also a small enough patch that we can > carry it internally, if you prefer that we don't touch this hack upstream. I'm fine with a change here. It could only possibly impact inclusions of regs.h from assembly, and should normally improve that, so the risk of breaking something is very low. We can always reassess if need be. We (AdaCore) have managed to work without fixincludes so far but will probably need to start relying on it in the near future. This will shake it some more. I wonder how we haven't hit the stop above, as it indicates an inclusion of regs.h without a prior inclusion of vxTypes from the VxWorks setjmp.h, included unconditionally from precompiled/stdc++.h via csetjmp. Maybe different set of headers for different versions of the OS. A comment would help and doesn't need to be big. The general idea, I think, is that different pieces of regs.h are useful in assembly or other contexts, the two contexts rely on slightly different sets of prerequisites and the more complete set (vxTypesOld.h) is incompatible with an inclusion in assembly for some configurations. Olivier ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE 2018-09-14 13:02 ` Olivier Hainque @ 2018-10-08 11:59 ` Rasmus Villemoes 0 siblings, 0 replies; 9+ messages in thread From: Rasmus Villemoes @ 2018-10-08 11:59 UTC (permalink / raw) To: Olivier Hainque; +Cc: GCC Patches, bkorb, Jérôme Lambourg On 2018-09-14 14:39, Olivier Hainque wrote: > > >> On 13 Sep 2018, at 00:25, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: > >>> What happens on your end if you just remove the hack ? > >> Unfortunately, the libstdc++ build breaks: >> >> In file included from >> /usr/powerpc-wrs-vxworks/wind_base/target/h/regs.h:66:0, >> from >> /bld/vestas/auto/work.20180912.214646/gcc-build/gcc/include-fixed/setjmp.h:57, >> from >> /bld/vestas/auto/work.20180912.214646/gcc-build/powerpc-wrs-vxworks/libstdc++-v3/include/csetjmp:42, >> from >> /bld/vestas/auto/work.20180912.214646/gcc-src/libstdc++-v3/include/precompiled/stdc++.h:42: >> /usr/powerpc-wrs-vxworks/wind_base/target/h/arch/ppc/regsPpc.h:33:5: >> error: 'UINT32' does not name a type >> UINT32 cr; /* condition register */ >> ^~~~~~ > > Ah, I see. Thanks for the experiment. > >> I'm happy to add an include of vxCpu in the _ASMLANGUAGE case, along >> with a big comment. But, it's also a small enough patch that we can >> carry it internally, if you prefer that we don't touch this hack upstream. > > I'm fine with a change here. It could only possibly impact inclusions > of regs.h from assembly, and should normally improve that, so the risk > of breaking something is very low. OK, I'll send an updated patch in a moment, adding the vxCpu include in the _ASMLANGUAGE case and keeping vxTypesOld out of _ASMLANGUAGE. > I wonder how we haven't hit the stop above, as it indicates an > inclusion of regs.h without a prior inclusion of vxTypes from the > VxWorks setjmp.h, included unconditionally from precompiled/stdc++.h > via csetjmp. Maybe different set of headers for different versions > of the OS. Yes, I believe other (newer) versions of VxWorks might have more self-contained headers, so they themselves pull in whatever other headers they rely on. Rasmus ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper 2018-06-27 14:27 [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE Rasmus Villemoes 2018-09-03 12:11 ` Olivier Hainque @ 2018-10-08 13:14 ` Rasmus Villemoes 2018-10-15 15:50 ` Olivier Hainque 1 sibling, 1 reply; 9+ messages in thread From: Rasmus Villemoes @ 2018-10-08 13:14 UTC (permalink / raw) To: gcc-patches Cc: Olivier Hainque, bkorb, Jérôme Lambourg, Rasmus Villemoes A quick experiment reveals that this hack is needed for C code - simply removing this hack entirely breaks the build of libstdc++, since regs.h (more accurately, the cpu-specific header it pulls in) defines structs in terms of types from vxTypesOld. Those definitions are properly guarded by #ifndef _ASMLANGUAGE, but the cpu-files do not take care to include vxTypesOld.h for the types they depend on. But when using regs.h from some assembly file, the assembler chokes on the typedefs in vxTypesOld.h. We can fix that by guarding the include of vxTypesOld by !_ASMLANGUAGE. This should not affect existing C code. Now, the OS' regs.h contains preprocessor conditionals such as #if CPU_FAMILY==I960 ... #endif /* CPU_FAMILY==I960 */ #if CPU_FAMILY==MC680X0 ... #endif /* CPU_FAMILY==MC680X0 */ Without definitions of CPU_FAMILY, I960 etc., these would all be true, which will not end well. Code using the fix-included regs.h automatically get vxCpu.h via a chain of includes from vxTypesOld.h, but we can make regs.h a little more self-contained for both C and asm users by doing an explicit include of vxCpu.h. ==changelog== fixincludes/ * inclhack.def (AAB_vxworks_regs_vxtypes): Add unconditional include of vxCpu.h, guard include of vxTypesOld.h by !_ASMLANGUAGE. * fixincl.x: Regenerate. --- fixincludes/inclhack.def | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def index f9ba9774f32..8fd9f7ef295 100644 --- a/fixincludes/inclhack.def +++ b/fixincludes/inclhack.def @@ -426,7 +426,17 @@ fix = { replace = <<- _EndOfHeader_ #ifndef _REGS_H #define _REGS_H + /* regs.h depends on CPU_FAMILY being properly defined, which + is done by vxCpu.h. */ + #include <types/vxCpu.h> + /* regs.h includes a CPU_FAMILY-specific header that requires + vxTypesOld.h to already have been included. Those headers + contain proper _ASMLANGUAGE guards around their typedefs, + but vxTypesOld.h itself does not. So we avoid including + vxTypesOld.h from assembly. */ + #ifndef _ASMLANGUAGE #include <types/vxTypesOld.h> + #endif #include_next <arch/../regs.h> #endif _EndOfHeader_; -- 2.19.1.3.g1d92a00e68 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper 2018-10-08 13:14 ` [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper Rasmus Villemoes @ 2018-10-15 15:50 ` Olivier Hainque 0 siblings, 0 replies; 9+ messages in thread From: Olivier Hainque @ 2018-10-15 15:50 UTC (permalink / raw) To: Rasmus Villemoes Cc: Olivier Hainque, gcc-patches, bkorb, Jérôme Lambourg Hi Rasmus, > On 8 Oct 2018, at 15:03, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote: > > fixincludes/ > > * inclhack.def (AAB_vxworks_regs_vxtypes): Add unconditional > include of vxCpu.h, guard include of vxTypesOld.h by > !_ASMLANGUAGE. > * fixincl.x: Regenerate. Good for me, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-15 15:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-27 14:27 [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE Rasmus Villemoes 2018-09-03 12:11 ` Olivier Hainque 2018-09-03 13:20 ` Rasmus Villemoes 2018-09-05 9:38 ` Olivier Hainque 2018-09-13 0:46 ` Rasmus Villemoes 2018-09-14 13:02 ` Olivier Hainque 2018-10-08 11:59 ` Rasmus Villemoes 2018-10-08 13:14 ` [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper Rasmus Villemoes 2018-10-15 15:50 ` Olivier Hainque
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).