Page MenuHomeFreeBSD
Differential D40944

arm64 lib32: prepare arm64 headers to redirect to arm
ClosedPublic
Actions

Authored by • karels on Jul 8 2023, 10:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 22, 8:17 PM
Unknown Object (File)
Tue, Apr 21, 8:31 PM
Unknown Object (File)
Tue, Apr 21, 11:28 AM
Unknown Object (File)
Sun, Apr 19, 10:30 AM
Unknown Object (File)
Tue, Apr 14, 2:01 PM
Unknown Object (File)
Tue, Apr 14, 6:10 AM
Unknown Object (File)
Mon, Apr 13, 9:01 PM
Unknown Object (File)
Mon, Apr 13, 9:00 PM
View All Files
Subscribers
emaste
markmi_dsl-only.net

Details

Reviewers
jhb
brooks
jrtc27
imp
andrew
manu
Commits
rGd5d97bed4ab6: arm64 lib32: prepare arm64 headers to redirect to arm
Summary

In order to compile lib32 libraries and other 32-bit code on arm64,
<machine/foo.h> needs to be redirected to an arm header rather
than arm64 when building with -m32. Ifdef the arm64 headers that
are installed in /usr/include/machine and used by user-level software
(including references from /usr/include/*.h) so that if arm is
defined when including the arm64 version, <arm/foo.h> is included
rather than using the rest of the file's contents. Some arm headers
had no arm64 equivalent; headers were added just to do the redirection.
These files use #error if arm is not defined to guard against
confusion. Also add an include/arm Makefile, and modify Makefiles
as needed to install everything, including the arm files in
/usr/include/arm.

The new arm64 headers are:

acle-compat.h
atomic-v6.h
cpu-v6.h
cpuinfo.h
pmap-v6.h
pte-v6.h
swi.h
sysreg.h

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jrtc27 added inline comments.Jul 12 2023, 6:24 PM
include/arm/Makefile
50

Yeah, procstat/zfs is a total mess that really needs to be redone... (https://github.com/CTSRD-CHERI/cheribsd/commit/68cc84debb0c275a364b065dab0aa06843602895 for some fun we had downstream)

54

Not sure if that one's such an XXX

61

I might hoist some of this into some common ../Makefile.libcompat post-commit, but this inter-file duplication is fine to commit for now IMO, feels less bad than intra-file

lib/msun/Makefile
163–164 ↗(On Diff #124512)

Best to avoid TARGET_ARCH checks outside top-level files

165 ↗(On Diff #124512)

Hm, this is because arm and aarch64 have different fenv.h, whilst x86 has a common one? The math.h one is going to be installed into arm/ and unused; this should be doing INCSDIR_fenv.h=.

lib/msun/aarch64/fenv.h
28

Missing blank line

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

?

sys/arm64/include/pte-v6.h
6 ↗(On Diff #124512)

?

sys/arm64/include/reloc.h
5

#else?

6

?

sys/arm64/include/swi.h
6 ↗(On Diff #124512)

?

sys/arm64/include/sysreg.h
7

?

andrew added a comment.Jul 12 2023, 9:26 PM

I'd like to review this, but won't have time in the next 1-2 weeks.

• karels added a comment.Jul 12 2023, 10:00 PM
In D40944#933422, @andrew wrote:

I'd like to review this, but won't have time in the next 1-2 weeks.

@andrew, Are you OK with the general scheme of doing this in the necessary arm64/include headers:

#ifdef arm
#include <arm/foo.h>
#else
/* rest of file */
#endif

If so, we can make second-order corrections later.

include/arm/Makefile
6

Sorry, that's a left-over from approach 1.

54

pte-v6.h is dead, right?

lib/msun/Makefile
165 ↗(On Diff #124512)

Correct. math.h needed to be in arm/ in approach 1. I guess math.h will be installed twice in the same place, but that's OK.

sys/arm64/include/reloc.h
5

This was a pre-existing empty file. Including it didn't cause an error in the past, so still should not.

imp added a comment.Jul 12 2023, 10:12 PM

The -v6.h is due to an incomplete removal of the v4/5 code that should have inlined these, but didn't.

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

Yea, arm/pmap.h includes arm/pmap-v6.h

sys/arm64/include/pte-v6.h
6 ↗(On Diff #124512)

arm/pmap_var.h includes this, along with cpu-v6.h

jrtc27 added inline comments.Jul 12 2023, 10:12 PM
lib/msun/Makefile
165 ↗(On Diff #124512)

Yeah, same as x86 and powerpc. Best to avoid confusion and not install unused headers.

sys/arm64/include/reloc.h
5

Ack, didn't notice that when skimming. I'd suggest:

#else
/* empty */
#endif

or similar (could ditch the error, but then might look like a mistake?) perhaps?

jrtc27 added inline comments.Jul 12 2023, 10:15 PM
include/arm/Makefile
54

v7 uses the same PTE format as v6 (as opposed to the v4 and v5 which shared the -v4 pmap bits). The only thing "dead" about it is that it doesn't need to be versioned any more.

sys/arm64/include/pmap-v6.h
6 ↗(On Diff #124512)

The ?'s here are for the blank lines specifically, not the files themselves

imp added inline comments.Jul 12 2023, 10:18 PM
include/arm/Makefile
54

Yea, one of the 'todo' items that never got done was inlining them back into the original files now that the need for them is gone.

• karels updated this revision to Diff 124608.Jul 13 2023, 2:14 PM

fix cosmetic problems, add fenv.h to include/arm/Makefile, move
include subdirectory creation into a loop to save a set of ifdefs

Harbormaster completed remote builds in B52616: Diff 124608.Jul 13 2023, 2:14 PM
• karels updated this revision to Diff 124610.Jul 13 2023, 3:48 PM
• karels marked 6 inline comments as done.

missed some blank lines, need /* empty */ comments

Harbormaster completed remote builds in B52617: Diff 124610.Jul 13 2023, 3:48 PM
jrtc27 added a comment.Jul 13 2023, 3:54 PM

add fenv.h to include/arm/Makefile

This is odd, what's that for?

include/Makefile
16

You can just SUBDIR+=${INCUDE_SUBDIRS} outside the ifs rather than duplicate each directory name. Though why not use the same variable name as Makefile.inc1?

• karels added a comment.Jul 13 2023, 4:04 PM
In D40944#933595, @jrtc27 wrote:

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

include/Makefile
16

I didn't want to assume that the source directory and the install directory had the same name. I didn't use the same name for INCLUDE_SUBDIRS because this is not necessarily related to LIBCOMPAT.

jrtc27 added a comment.Jul 13 2023, 4:37 PM
In D40944#933600, @karels wrote:
In D40944#933595, @jrtc27 wrote:

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

That doesn't sound right. install${libcompat} recurses on _lc_install which then recurses into lib, which should in turn recurse into lib/msun.

• karels added a comment.Jul 13 2023, 5:07 PM
In D40944#933612, @jrtc27 wrote:
In D40944#933600, @karels wrote:
In D40944#933595, @jrtc27 wrote:

add fenv.h to include/arm/Makefile

This is odd, what's that for?

fenv.h wasn't getting installed in /usr/include/arm, just ${WORLDTMP}/usr/include/arm. lib/msun/Makefile doesn't get invoked with a LIBCOMPAT run during installworld.

That doesn't sound right. install${libcompat} recurses on _lc_install which then recurses into lib, which should in turn recurse into lib/msun.

I don't know, but the lib32 section of msun install output has only this:

===> msun (install)
install  -C -o root -g wheel -m 444   libm.a /scratch/arm64/usr/lib32/
install  -s -o root -g wheel -m 444   -S  libm.so.5 /scratch/arm64/usr/lib32/
install  -o root -g wheel -m 444    libm.so.5.debug /scratch/arm64/usr/lib/debug
/usr/lib32/
install -l rs -o root -g wheel -m 755 -S   libm.so.5 /scratch/arm64/usr/lib32/li
bm.so
===> libsqlite3 (install)

The non-compat install also installs headers (aarch64 for fenv.h), man pages, etc.

jrtc27 added a comment.Jul 13 2023, 5:10 PM

Oh, LIB${LIBCOMPAT}IMAKE has MK_INCLUDES=no, hmm, this is more of a mess than I thought :(

jrtc27 added a comment.Jul 13 2023, 5:16 PM

My instinct is MK_INCLUDES=no should be in LIB${LIBCOMPAT}WMAKE not ...IMAKE so that we get a consistent set of headers for ${WORLDTMP} and the installed world, but who knows if that breaks something... I'll go look at that with a tinderbox later

jrtc27 added a comment.Jul 13 2023, 10:03 PM

D41030

• karels marked 3 inline comments as done.Jul 14 2023, 12:45 PM
In D40944#933866, @jrtc27 wrote:

D41030

The lib32/arm build still seems to work with this change, assuming the fenv.h change to include/arm/Makefile.

I think the review comments are all addressed now. Anyone have any remaining comments? On this or the other reviews in the series?

• karels added a comment.Jul 16 2023, 4:12 PM

Where do we stand on this review? Any more comments or approvals? Do we need to wait for @andrew ?

• karels added a comment.Jul 19 2023, 8:03 PM
In D40944#934594, @karels wrote:

Where do we stand on this review? Any more comments or approvals? Do we need to wait for @andrew ?

I'll note that we're down to just over 2 weeks until 14.0 slush. I'd really like to get this into a build in time to fix it before slush, but that time is approximately now. Several people approved the approach in email; should I commit this on that basis?

andrew added inline comments.Jul 20 2023, 11:00 AM
sys/arm64/include/atomic-v6.h
1 ↗(On Diff #124610)

Can we merge the arm atomic-v6.h and atomic.h?

sys/arm64/include/cpu-v6.h
1 ↗(On Diff #124610)

Do we need this file? On arm it's only usable by the kernel.

sys/arm64/include/cpuinfo.h
2

Do we need this file? It looks like it's kernel specific.

sys/arm64/include/pte-v6.h
1 ↗(On Diff #124610)

Is this file used by userspace?

sys/arm64/include/swi.h
1 ↗(On Diff #124610)

Is this file used by userspace? If not I don't think it should exist on arm.

sys/arm64/include/sysreg.h
2

Do we need this file? It looks like it's kernel specific.

• karels added a comment.Jul 20 2023, 11:49 AM

I'll double-check on the individual files you asked about in an hour or so, but in summary: I didn't add headers unless they were included, directly or indirectly, via files in <sys/>, or were included by something in the build. I wasn't really considering any merges, etc, as part of the same change.

• karels added inline comments.Jul 20 2023, 1:48 PM
sys/arm64/include/atomic-v6.h
1 ↗(On Diff #124610)

In theory this can be done. I wouldn't want to do it in this commit, as it is a fairly large change (or at least, a lot of code moves).

sys/arm64/include/cpu-v6.h
1 ↗(On Diff #124610)

It's included by <machine/cpu.h>, which in turn is included by <sys/proc.h>.

sys/arm64/include/cpuinfo.h
2

Included by cpu-v6.h.

sys/arm64/include/pte-v6.h
1 ↗(On Diff #124610)

yes, it is directly included from libkvm.

sys/arm64/include/swi.h
1 ↗(On Diff #124610)

It's used by SYS.h for arm libc.

sys/arm64/include/sysreg.h
2

It is included (#ifndef _KERNEL) by arm's atomic.h.

jhb added a comment.Jul 20 2023, 5:47 PM

I think that post-v4/v5 removal cleanups should be done separately and not required as a blocker for this change (e.g. merging pmap.h and pmap-v6.h)

• karels added a comment.Jul 21 2023, 12:00 PM

@andrew, any chance you can approve this?

andrew added a comment.Jul 21 2023, 12:44 PM

See D41133 to D41140 for arm header cleanups.

• karels added a comment.Jul 24 2023, 1:04 AM
In D40944#936659, @andrew wrote:

See D41133 to D41140 for arm header cleanups.

For anyone who hasn't looked at these, they eliminate or combine some of the headers like atomic-v6.h. This raises the question of ordering of the two sets of commits. As much as I'd like to close out the lib32 changes, it makes more sense to have @andrew's go in first so new headers don't get created and them removed immediately.

jhb added a comment.Jul 24 2023, 7:39 PM
In D40944#937134, @karels wrote:
In D40944#936659, @andrew wrote:

See D41133 to D41140 for arm header cleanups.

For anyone who hasn't looked at these, they eliminate or combine some of the headers like atomic-v6.h. This raises the question of ordering of the two sets of commits. As much as I'd like to close out the lib32 changes, it makes more sense to have @andrew's go in first so new headers don't get created and them removed immediately.

Agreed on the ordering, but it also seems like Andrew's has landed now.

imp accepted this revision.Jul 24 2023, 9:23 PM

Now that Andrew's changes have landed this is good

This revision is now accepted and ready to land.Jul 24 2023, 9:23 PM
• karels updated this revision to Diff 125107.Jul 24 2023, 11:47 PM

Integrate with andrew's changes, remove added headers no longer needed.

This revision now requires review to proceed.Jul 24 2023, 11:47 PM
Harbormaster completed remote builds in B52811: Diff 125107.Jul 24 2023, 11:47 PM
markmi_dsl-only.net added a subscriber: markmi_dsl-only.net.Jul 25 2023, 12:13 AM

The summary's "new arm64 headers are" list is out of date and should not be blindly copied into commit notes.

• karels added a comment.Jul 25 2023, 12:50 AM
In D40944#937441, @markmi_dsl-only.net wrote:

The summary's "new arm64 headers are" list is out of date and should not be blindly copied into commit notes.

The list is correct in git, but Phabricator doesn't update it. The list is:

acle-compat.h
cpuinfo.h
sysreg.h

Andrew reduced the use of sysreg.h, but cpu.h includes it, and libprocstat/zfs uses cpu.h.

• karels added a comment.Jul 25 2023, 10:23 PM

Any comments? I'd like to get this into this week's build. The only changes are to delete a number of the new wrapper files, and add one new redirection (pte.h instead of pte-v6.h).

jrtc27 added a comment.Jul 25 2023, 10:29 PM

Thinking about this more, would the dummy headers not be better off in include/arm/?

jrtc27 requested changes to this revision.Jul 25 2023, 10:31 PM
jrtc27 added inline comments.
include/arm/Makefile
60–61
lib/msun/Makefile
163 ↗(On Diff #125107)

This code should now be dead; Makefile.libcompat now does all builds with MK_INCLUDES=no

This revision now requires changes to proceed.Jul 25 2023, 10:31 PM
• karels added a comment.Jul 25 2023, 10:34 PM
In D40944#937930, @jrtc27 wrote:

Thinking about this more, would the dummy headers not be better off in include/arm/?

You mean the headers that are just wrappers? They are included as <machine/foo.h>, so need to be in include/machine.

jrtc27 added a comment.Jul 25 2023, 10:35 PM
In D40944#937936, @karels wrote:
In D40944#937930, @jrtc27 wrote:

Thinking about this more, would the dummy headers not be better off in include/arm/?

You mean the headers that are just wrappers? They are included as <machine/foo.h>, so need to be in include/machine.

Uh, right, yes, disregard that then

• karels updated this revision to Diff 125160.Jul 25 2023, 11:33 PM

Address review comments.

Harbormaster completed remote builds in B52843: Diff 125160.Jul 25 2023, 11:33 PM
• karels marked 2 inline comments as done.Jul 25 2023, 11:37 PM
jrtc27 accepted this revision.Jul 25 2023, 11:45 PM

Oops, missed one nit, but can be fixed at commit time. Otherwise looks ok to me.

sys/arm64/include/acle-compat.h
6–7
This revision is now accepted and ready to land.Jul 25 2023, 11:45 PM
Closed by commit rGd5d97bed4ab6: arm64 lib32: prepare arm64 headers to redirect to arm (authored by • karels). · Explain WhyJul 26 2023, 12:01 AM
This revision was automatically updated to reflect the committed changes.
• karels added a commit: rGd5d97bed4ab6: arm64 lib32: prepare arm64 headers to redirect to arm.

Revision Contents
Changeset List

PathSize
include/
13 lines
arm/
60 lines
lib/
msun/
aarch64/
6 lines
sys/
arm64/
include/
6 lines
6 lines
6 lines
6 lines
6 lines
5 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
5 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
6 lines
5 lines
6 lines
6 lines
6 lines
6 lines
6 lines

Diff 125163

View Options

include/Makefile

Loading...
View Options

include/arm/Makefile

Loading...
View Options

lib/msun/aarch64/fenv.h

Loading...
View Options

sys/arm64/include/_align.h

Loading...
View Options

sys/arm64/include/_inttypes.h

Loading...
View Options

sys/arm64/include/_limits.h

Loading...
View Options

sys/arm64/include/_stdint.h

Loading...
View Options

sys/arm64/include/_types.h

Loading...
View Options

sys/arm64/include/acle-compat.h

Loading...
View Options

sys/arm64/include/armreg.h

Loading...
View Options

sys/arm64/include/asm.h

Loading...
View Options

sys/arm64/include/atomic.h

Loading...
View Options

sys/arm64/include/bus.h

Loading...
View Options

sys/arm64/include/counter.h

Loading...
View Options

sys/arm64/include/cpu.h

Loading...
View Options

sys/arm64/include/cpufunc.h

Loading...
View Options

sys/arm64/include/cpuinfo.h

Loading...
View Options

sys/arm64/include/efi.h

Loading...
View Options

sys/arm64/include/elf.h

Loading...
View Options

sys/arm64/include/exec.h

Loading...
View Options

sys/arm64/include/float.h

Loading...
View Options

sys/arm64/include/frame.h

Loading...
View Options

sys/arm64/include/ieeefp.h

Loading...
View Options

sys/arm64/include/param.h

Loading...
View Options

sys/arm64/include/pcb.h

Loading...
View Options

sys/arm64/include/pcpu.h

Loading...
View Options

sys/arm64/include/pcpu_aux.h

Loading...
View Options

sys/arm64/include/pmap.h

Loading...
View Options

sys/arm64/include/proc.h

Loading...
View Options

sys/arm64/include/profile.h

Loading...
View Options

sys/arm64/include/pte.h

Loading...
View Options

sys/arm64/include/reg.h

Loading...
View Options

sys/arm64/include/reloc.h

Loading...
View Options

sys/arm64/include/resource.h

Loading...
View Options

sys/arm64/include/runq.h

Loading...
View Options

sys/arm64/include/setjmp.h

Loading...
View Options

sys/arm64/include/sf_buf.h

Loading...
View Options

sys/arm64/include/signal.h

Loading...
View Options

sys/arm64/include/sysarch.h

Loading...
View Options

sys/arm64/include/sysreg.h

Loading...
View Options

sys/arm64/include/tls.h

Loading...
View Options

sys/arm64/include/ucontext.h

Loading...
View Options

sys/arm64/include/vdso.h

Loading...
View Options

sys/arm64/include/vfp.h

Loading...
View Options

sys/arm64/include/vmparam.h

Loading...