Page MenuHomeFreeBSD
Differential D8460

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

Authored by sgalabov on Nov 7 2016, 9:50 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Feb 7, 3:07 PM
Unknown Object (File)
Mon, Feb 3, 10:30 AM
Unknown Object (File)
Mon, Jan 20, 10:37 AM
Unknown Object (File)
Mon, Jan 20, 10:37 AM
Unknown Object (File)
Jan 13 2025, 10:30 AM
Unknown Object (File)
Nov 24 2024, 9:28 PM
Unknown Object (File)
Oct 23 2024, 9:42 PM
Unknown Object (File)
Oct 3 2024, 11:54 PM
View All 80 Files
Subscribers
andrew
imp
mmel

Details

Reviewers
andrew
mmel
Group Reviewers
ARM
Commits
rS308415: Generate an error if machine/armreg.h is included without sys/cdefs.h
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 - subversion
Lint
Lint Not Applicable
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 - subversion.
sgalabov added a project: ARM.
Herald added subscribers: andrew, imp. · View Herald TranscriptNov 7 2016, 9:50 AM
andrew added a comment.Nov 7 2016, 10:03 AM

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.

sgalabov added a comment.Nov 7 2016, 10:11 AM
In D8460#175936, @andrew wrote:

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.

sgalabov added a comment.Nov 7 2016, 10:13 AM
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?

sgalabov added a comment.Nov 7 2016, 10:17 AM

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.

sgalabov added a comment.Nov 7 2016, 10:37 AM

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
sgalabov added a comment.Nov 7 2016, 10:58 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
andrew added a comment.Nov 7 2016, 11:02 AM

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

andrew added a comment.Nov 7 2016, 11:07 AM
In D8460#175966, @andrew wrote:

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.
In D8460#175967, @andrew wrote:
In D8460#175966, @andrew wrote:

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
Closed by commit rS308415: Generate an error if machine/armreg.h is included without sys/cdefs.h (authored by sgalabov). · Explain WhyNov 7 2016, 11:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
arm/
include/
4 lines

Diff 22050

View Options

head/sys/arm/include/armreg.h

Loading...