mips: hide regnum definitions behind _KERNEL/_WANT_MIPS_REGNUM
ClosedPublic
Actions

Authored by kevans on Aug 20 2019, 12:08 AM.

Details

Reviewers

imp
jhb

Commits

rS351409: mips: hide regnum definitions behind _KERNEL/_WANT_MIPS_REGNUM
rS351408: libsa: mips: use _JB_* from machine/asm.h, remove regnum dep
rS351407: libc: mips: remove unused longjmp.c

Summary

machine/regnum.h ends up being included by sys/procfs.h and sys/ptrace.h via machine/reg.h. Many of the regnum definitions are too short and too generic to be exposing to any userland application including one of these too headers. Moreover, these actively cause build failures in googletest (template <typename T1 ...> expanding to template <typename 9 ...>).

Hide the definitions behind _KERNEL or _WANT_MIPS_REGNUM, and patch all of the userland consumers to define as needed.

Discussed with: imp, jhb

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 26018

Event Timeline

kevans created this revision.Aug 20 2019, 12:08 AM

Herald added a subscriber: emaste. · View Herald TranscriptAug 20 2019, 12:08 AM

Harbormaster completed remote builds in B25966: Diff 61020.Aug 20 2019, 12:08 AM

kevans edited the summary of this revision. (Show Details)Aug 20 2019, 12:08 AM

imp accepted this revision.Aug 20 2019, 2:52 AM

This revision is now accepted and ready to land.Aug 20 2019, 2:52 AM

jhb added inline comments.Aug 20 2019, 9:24 PM

lib/libc/mips/gen/_setjmp.S
37	So where does this file actually use these? Everything in this file looks like it uses <machine/asm.h> bits, but not regnum bits? If you happen to have the build log failure handy that would be nice is all. Otherwise I guess I can try to reproduce. I don't see any uses of regnum.h constants in the other asm files either fwiw, or some of the C files.

kevans added inline comments.Aug 20 2019, 9:30 PM

lib/libc/mips/gen/_setjmp.S
37	Admittedly, these that directly included <machine/regnum.h> I preemptively added _WANT_MIPS_REGNUM when I didn't see either of the two NUM* constants I left outside of _KERNEL \|\| _WANT_MIPS_REGNUM. I'll go back through and audit these, removing machine/regnum.h as necessary...

Revisit previous assumptions... some of these didn't need <machine/regnum.h> at all, so just remove them to make it more clear to grep where regnum.h is used. I can commit these separately if we care enough.

This revision now requires review to proceed.Aug 20 2019, 9:59 PM

Harbormaster completed remote builds in B25989: Diff 61066.Aug 20 2019, 9:59 PM

kevans marked an inline comment as done.Aug 22 2019, 2:09 PM

jhb added inline comments.Aug 22 2019, 5:05 PM

lib/libc/mips/gen/longjmp.c
0	I actually think this entire file can be removed.
stand/libsa/mips/_setjmp.S
60–61	Ugh, not your fault and not to be fixed right now, but the _JB_* constants in machine/asm.h need to be fixed, and then the setjmp routines should use those instead of regnum.h I think. We don't even use the same magic number in these files as _JB_MAGIC constants (sigh).
sys/mips/include/regnum.h
169–170	I think you can kill this entirely (can't find it used in the kernel or libc)

kevans added inline comments.Aug 22 2019, 5:33 PM

stand/libsa/mips/_setjmp.S
60–61	Admittedly, it's not clear to me why this is so divergent from the _setjmp.S in libc -- which actually does use the _JB_ constants and seems to actually line up with the buffer described in machine/asm.h.

jhb added inline comments.Aug 22 2019, 6:46 PM

stand/libsa/mips/_setjmp.S
60–61	Wait, I missed this was in libsa and not libc. I would totally be down with fixing this to match libc. A question of how much work you want to do. :)

imp added inline comments.Aug 22 2019, 6:48 PM

stand/libsa/mips/_setjmp.S
60–61	I thought I was careful when I imported juniper mips to mop all this up (though several cases slipped through that have been fixed over the years).... but I didn't mop up libsa since it wasn't there at the time... that was done well after I imported things...

Delete ^/lib/libc/mips/gen/longjmp.c entirely
Completely remove NREGS; it's unused (buildworld + buildkernel confirms)
Fix libsa _setjmp to use _JB_REG constants and match libc implementation, removes dependency on <machine/regnum.h> completely.

Harbormaster completed remote builds in B26018: Diff 61126.Aug 22 2019, 6:49 PM

I'm cool, though I'd like to think about having mips share more of the .S's with libc and have good ifdefs there.

imp accepted this revision.Aug 22 2019, 7:00 PM

This revision is now accepted and ready to land.Aug 22 2019, 7:00 PM

jhb accepted this revision.Aug 22 2019, 8:33 PM

Closed by commit rS351407: libc: mips: remove unused longjmp.c (authored by kevans). · Explain WhyAug 22 2019, 9:41 PM

This revision was automatically updated to reflect the committed changes.

kevans added a commit: rS351407: libc: mips: remove unused longjmp.c.

kevans added a commit: rS351408: libsa: mips: use _JB_* from machine/asm.h, remove regnum dep.

kevans added a commit: rS351409: mips: hide regnum definitions behind _KERNEL/_WANT_MIPS_REGNUM.Aug 22 2019, 9:43 PM