Page MenuHomeFreeBSD

mips: hide regnum definitions behind _KERNEL/_WANT_MIPS_REGNUM
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kevans created this revision.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 ↗(On Diff #61020)

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 ↗(On Diff #61020)

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...

kevans updated this revision to Diff 61066.Aug 20 2019, 9:59 PM

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
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
35 ↗(On Diff #61066)

I actually think this entire file can be removed.

stand/libsa/mips/_setjmp.S
63 ↗(On Diff #61066)

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
170 ↗(On Diff #61066)

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
63 ↗(On Diff #61066)

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
63 ↗(On Diff #61066)

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
63 ↗(On Diff #61066)

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...

kevans updated this revision to Diff 61126.Aug 22 2019, 6:49 PM
  • 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.
imp added a comment.Aug 22 2019, 6:56 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
This revision was automatically updated to reflect the committed changes.