Page MenuHomeFreeBSD

ARM: include machine/acle-compat.h in armreg.h if __ARM_ARCH is not defined
ClosedPublic

Authored by sgalabov on Nov 7 2016, 9:50 AM.

Details

Summary

In cases where ARM_ARCH is not defined, include machine/acle-compat.h in armreg.h so that compilation doesn't fail due to an undefined ARM_ARCH macro.

One such case at the moment is the SHEEVAPLUG kernel (possibly other Marvell kirkwood/orion based systems would be the same).

This change, together with D8459, allow us to build a SHEEVAPLUG kernel that seems to be working fine from the limited testing so far.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sgalabov updated this revision to Diff 22046.Nov 7 2016, 9:50 AM
sgalabov retitled this revision from to ARM: include machine/acle-compat.h in armreg.h if __ARM_ARCH is not defined.
sgalabov updated this object.
sgalabov edited the test plan for this revision. (Show Details)
sgalabov added a reviewer: ARM.
sgalabov set the repository for this revision to rS FreeBSD src repository.
sgalabov added a project: ARM.

When is it the case that __ARM_ARCH is undefined? machine/acle-compat.h is included from sys/cdefs.h so this should have already been included.

mmel added a subscriber: mmel.Nov 7 2016, 10:08 AM

Are you sure? Also for D8459? I'm able to compile actual SHEEVAPLUG kernel without any issue.

When is it the case that __ARM_ARCH is undefined? machine/acle-compat.h is included from sys/cdefs.h so this should have already been included.

When compiling a SHEEVAPLUG kernel with
make TARGET_ARCH=arm KERNCONF=SHEEVAPLUG buildkernel

compilation fails due to __ARM_ARCH not being defined within machine/armreg.h, which is included from several files, such as sys/arm/arm/cpufunc_asm_sheeva.S...
Seeing mmel@'s comment in the meantime - am I compiling things incorrectly? I am using current from git as of today.

In D8460#175937, @meloun-miracle-cz wrote:

Are you sure? Also for D8459? I'm able to compile actual SHEEVAPLUG kernel without any issue.

How do you compile and how recent is your snapshot of current? I am using today's current from github and compilation is failing... maybe I am not using the "right" way to build?

All files, which report an issue of __ARM_ARCH not being defined (and include machine/armreg.h):
sys/arm/arm/cpufunc_asm_sheeva.S
sys/arm/arm/fiq_subr.S
sys/arm/arm/setstack.s

mmel added a comment.Nov 7 2016, 10:21 AM

I just compile it again, with

make -j8 buildkernel TARGET=arm TARGETARCH=arm KERNCONF=SHEEVAPLUG

...

>>> Kernel build for SHEEVAPLUG completed on Mon Nov 7 11:17:27 CET 2016

I'm not sure if TARGETARCH=arm is necessary, but I have 'setenv TARGET_ARCH armv6' in my login script.

Hmm, this is weird, it still fails the same way for me, although I am using the default gcc:
gcc version 4.2.1 20070831 patched [FreeBSD]

mmel accepted this revision.Nov 7 2016, 10:51 AM
mmel added a reviewer: mmel.

It's clear now. clang (and and any newer gcc) defines symbols from acle-compat.h by itself, implicitly. Unfortunately, outdated gcc from base not.

This revision is now accepted and ready to land.Nov 7 2016, 10:51 AM

Any other comments on this? If it's ok with everyone I'd like to commit it...

andrew requested changes to this revision.Nov 7 2016, 10:59 AM
andrew added a reviewer: andrew.

I'm about to commit the correct fix of including armreg.h after asm.h.

This revision now requires changes to proceed.Nov 7 2016, 10:59 AM

Can you try with rS308408. It might pay to add an error if this file is included but _MACHINE_ASM_H_ is undefined.

Can you try with rS308408. It might pay to add an error if this file is included but _MACHINE_ASM_H_ is undefined.

Even better would be _SYS_CDEFS_H_.

sgalabov updated this revision to Diff 22049.Nov 7 2016, 11:14 AM
sgalabov edited edge metadata.

Can you try with rS308408. It might pay to add an error if this file is included but _MACHINE_ASM_H_ is undefined.

Even better would be _SYS_CDEFS_H_.

This works now. I've updated the diff to test whether _SYS_CDEFS_H_ is defined within armreg.h and, if not, throw an #error.

andrew accepted this revision.Nov 7 2016, 11:30 AM
andrew edited edge metadata.
This revision is now accepted and ready to land.Nov 7 2016, 11:30 AM
This revision was automatically updated to reflect the committed changes.