On 16/05/2025 17:11, Mark Johnston wrote:
On Fri, May 16, 2025 at 03:22:52PM +0300, Andriy Gapon wrote:
On 27/04/2025 17:26, Mark Johnston wrote:
On Thu, Apr 24, 2025 at 08:56:44AM +0300, Andriy Gapon wrote:
On 23/04/2025 21:56, Andriy Gapon wrote:
BTW, I've been wondering how illumos avoids the problem even though they
do not use any special dlopen flags.
It turns out that they link almost all system shared libraries with
-Bdirect option (which is Solaris/illumos specific).
It's somewhat similar to, but different from, -Bsymbolic.
https://docs.oracle.com/cd/E23824_01/html/819-0690/aehzq.html#scrolltoc
https://docs.oracle.com/cd/E36784_01/html/E36857/gejfe.html
Oh, and it looks like there is an even better explanation for illumos.
There is a version map file for libdtrace which explicitly lists API
functions and makes everything else local.
https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libdtrace/common/mapfile-vers
I wonder why we didn't do the same when porting.
Maybe we should do that now?
I don't have any objection, but I believe adding a version map after the
fact doesn't accomplish much, assuming that we care deeply about ABI
stability in libdtrace.so (I'm not sure we do though).
My primary goal here is not ABI stability, but hiding symbols that really
should not be exported. See more at the end.
At the same time I am not sure why it could be too late to start caring
about ABI stability now. Assuming we actually would want to do that.
I just mean that the version map helps only helps provide stability for
binaries linked to libdtrace.so after the version map is introduced.
So, if we introduce it (and bump the library version as kib pointed out), then
eventually it becomes useful (like it did for all other libraries that we
versioned).
And I don't want to single out libtrace here.
It seems that the story is the same for all libraries that have been ported
from illumos.
E.g., libzfs_core was supposed to be a library that cares greatly about its
API / ABI stability.
I think that on FreeBSD we should use symbol visibility attributes or a
symbol map to hide (make local) symbols that are not expected to be
interposed or have a high chance to be interposed by accident.
IMO, yyparse should definitely get that treatment.
I think that approach would be better than magic rtld tricks.
Especially because the tricks do not work with the current rtld.
I'd rather make a change to libdtrace.so than to rtld.
This, while not as nice as the illumos solution, fixes my specific issue:
diff --git a/cddl/lib/libdtrace/Makefile b/cddl/lib/libdtrace/Makefile
index d086fffb07bc..58054d129b49 100644
--- a/cddl/lib/libdtrace/Makefile
+++ b/cddl/lib/libdtrace/Makefile
@@ -146,7 +146,8 @@ CFLAGS+= -fsanitize=address -fsanitize=undefined
LDFLAGS+= -fsanitize=address -fsanitize=undefined
.endif
-LIBADD= ctf elf proc pthread rtld_db xo
+VERSION_MAP= ${.CURDIR}/Symbol.map
+LIBADD= ctf elf proc pthread rtld_db xo
CLEANFILES= dt_errtags.c dt_names.c
diff --git a/cddl/lib/libdtrace/Symbol.map b/cddl/lib/libdtrace/Symbol.map
new file mode 100644
index 000000000000..89ee9de65209
--- /dev/null
+++ b/cddl/lib/libdtrace/Symbol.map
@@ -0,0 +1,4 @@
+{
+ local:
+ yy*;
+};
This just gives the lexer/parser symbols in libdtrace.so local
visibility? I think that's fine.
Yes, that's the intention.
I tested locally two versions of Symbol.map for libdtrace.
The basic one quoted here and a more extended one based on illumos
lib/libdtrace/common/mapfile-vers.
The latter version does not define any symbol versions, its purpose is only
to be a white-list of things to make public / global:
https://people.freebsd.org/~avg/libdtrace-Symbol.map
Do we really want to export _dtrace_debug?
Hmm, I don't know, that came from illumos.
However, I do not see any references to _dtrace_debug outside of libdtrace
neither in FreeBSD nor in illumos.
So, I guess that it can be removed.
Maybe it was exposed for potential libdtrace consumers that might want to enable
debug directly rather than via environment.
Comparing to illumos I only had to add 3 dtrace_oformat* symbols,
Both versions worked equally well in my tests, but maybe I missed more of
FreeBSD extensions.
Which one would be better to get into the tree?
Having a full whitelist seems preferable to me. Did you test lockstat
as well? I believe it and dtrace(8) are the only users of libdtrace.so
in the base system.
I didn't before, I've just done now, it works.
I'll post a review request over the weekend (hopefully).
LD_PRELOAD=/usr/obj/amd64.amd64/cddl/lib/libdtrace/libdtrace.so.2 lockstat -H -D
10 -x aggsize=100m -l smp_ipi_mtx -s 20 -P sleep 5
Spin lock hold: 106 events in 5.021 seconds (21 events/sec)
-------------------------------------------------------------------------------
Count indv cuml rcnt nsec Lock Caller
11 53% 53% 0.00 99541 smp rendezvous
__mtx_unlock_spin_flags+0xe3
nsec ------ Time Distribution ------ count Stack
16384 |@@ 1 smp_rendezvous_cpus+0x187
32768 |@@@@@@@@@@@@@@@@@@@ 7 dtrace_sync+0x39
65536 | 0 dtrace_state_deadman+0x13
131072 | 0 softclock_call_cc+0x1d9
262144 |@@ 1 softclock_thread+0xc6
524288 |@@@@@ 2 fork_exit+0x82
0xffffffff81030a3e
-------------------------------------------------------------------------------
...
--
Andriy Gapon
