dtrace: do not overload libproc flags

[vangyzen]

Summary

dtrace stored its PR_RLC and PR_KLC flags in the proc_handle's flags, where they collided with PATTACH_FORCE and PATTACH_RDONLY, respectively. Thus, Psetflags(PR_KLC) effectively also set the PATTACH_RDONLY flag.

Since the flags are private to dtrace (at least on FreeBSD), store them in dtrace's own dt_proc structure instead.

MFC after: 1 week
Sponsored by: Dell EMC Isilon

Test Plan

All libproc tests passed. There was no change in the dtrace test results.
The change in D41122 works, which required this change.

[markj]

cddl/compat/opensolaris/include/libproc.h

37–38

39

How do these flags get implemented? When I looked at the previous review I assumed that these mapped onto native libproc flags, but now they clearly don't.

[vangyzen]

cddl/compat/opensolaris/include/libproc.h

39

DTrace implements these flags itself, entirely in dt_proc.c. It stores them in the proc_handle, I assume for Solaris compatibility, but they don't go any farther into libproc. On FreeBSD, I consider that an abuse, but fixing it would diverge even more from upstream.

[vangyzen]

• fix comment style

[vangyzen]

What do you think, @markj?

[markj]

cddl/compat/opensolaris/include/libproc.h

39

I think diverging from upstream here is not problematic, given 1) this code basically never changes upstream and we already have quite a lot of FreeBSD-specific bits, 2) this code is quite a lot harder to understand otherwise. In general I would welcome changes which simplify libdtrace's process handling.

[vangyzen]

cddl/compat/opensolaris/include/libproc.h

39

Sounds good to me. I can spend some time on that.

In my opinion, the first step is unifdef illumos. Otherwise, any attempt to simplify just makes things worse. Are we on the same page?

[markj]

cddl/compat/opensolaris/include/libproc.h

39

I think that's reasonable. To be clear, I'm not suggesting unifdefing all of libdtrace, just the code that interacts with libproc.

[vangyzen]

cddl/compat/opensolaris/include/libproc.h

39

Cool. D41173 does that. I'll rebase this onto that shortly.

[vangyzen]

• rebase onto D41173; store flags in dt_proc

[markj]

Thanks, this version is much clearer IMO.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_proc.c

682

Currently we never use this state - do you see any way for this case to happen with the old code?

692

I think this default case is redundant since you've covered all of the enum cases.

[vangyzen]

cddl/contrib/opensolaris/lib/libdtrace/common/dt_proc.c

682

I don't. I'll rip it out.

692

True, but if dpr_close is corrupted at runtime, this will catch it early and loudly. It's also a documenting assertion that all cases are covered.

[markj]

cddl/contrib/opensolaris/lib/libdtrace/common/dt_proc.c

692

AFAIU __unreachable() is a hint to the compiler that "this code path cannot be reached, so optimize accordingly." It will not (by itself) cause the code to fail closed in the case of memory corruption.

To get the effect you want, I'd expect you need to 1) cast the value to an int, 2) use a traditional assert(0).

[vangyzen]

cddl/contrib/opensolaris/lib/libdtrace/common/dt_proc.c

692

In the past, there was some directive that would emit a ud2 instruction (on amd64). However, you're right (of course), this presently emits nothing. I'll just remove it.

[vangyzen]

• remove DT_CLOSE_HANG, which never happens on FreeBSD