[PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[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

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Olivier Hainque]

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

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Rasmus Villemoes]

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

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Olivier Hainque]

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 ?

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Rasmus Villemoes]

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

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Olivier Hainque]

> 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

Re: [PATCH] fixincludes: vxworks: regs.h: Guard include of vxTypesOld.h by !_ASMLANGUAGE

[Rasmus Villemoes]

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

[PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper

[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

Re: [PATCH v2] fixincludes: vxworks: regs.h: Fix includes in regs.h wrapper

[Olivier Hainque]

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.