bsd.lib.mk: add targets to detect symbol changes

[brooks]

Summary

check-symbols: Diffs the set of symbols and versions in the library

		against a reference list (which is per-MACHINE_ARCH
		if SYMBOLS_REF_MD is defined).

update-symref: Updates the copy of the symbol reference in the source

		tree.

If symref files are committed to the tree or generated before making a
potentially disruptive change, check-symbols ensures the symbol-level
ABI changes are tracked and well understood. This is somewhat redundant
to linking with --no-undefined-version, but it works for unversioned
libraries and detects list symbols exposed by the sym_compat or
sym_default macros.

check-symbols can be called as a top-level target and traversers
subdirs. update-symref can not currently as if creates files in ways
that may be undesierable (e.g. in the tests/stdlib/dynthr_mod
subdirectory when invoked in lib/libc).

The current implementation is built on a messy set of shell tools
parsing objdump output and diff for comparison. It would benefit from
being rewritten in lua and being adjusted to provide more granular
feedback.

[brooks]

While working on the libc/sys split and all the reviews I'm stacking under D44216, I realized I needed some automation to make sure I wasn't accidentally removing symbols. This is the result. I think we should probably commit symref files to the tree for key libraries and maybe all versioned libraries as my perusal of history suggests we lose symbols by accident with some regularity.

[brooks]

Remove stray depend from a prior experiment

[brooks]

share/mk/bsd.lib.mk

392

There's some argument this shouldn't include ${SHLIB_NAME} and maybe they should be dot files to keep them out of the way in the normal course of things.

[brooks]

I'm slightly ahead of myself with this change. It works fine with a native build, but needs a bootstrapped objdump in the general case. I'l sort that out in a bit.

[kib]

Are you aware of devel/abi-compliance-checker? The version in ports is rotten, https://lvc.github.io/abi-compliance-checker/ seems to claim that FreeBSD is supported.

[sjg]

FWIW we have sym and abi checkers in junos build, they are each in a separate makefile included by bsd.lib.mk if the appropriate knob is enabled
The sym checker does BSS and TLS symbol checks, not directly useful here, but just a thought

[brooks]

I'm definitely open to supporting more complete checkers, but my goal is to have something in tree and enabled by default. I worry that out of tree things lead to the maintenance burden being in the wrong place as developers commit changes that haven't been checked (at least until some future day when the project has more complete pre-commit CI).

[kib]

In D44271#1010069, @brooks wrote:

I'm definitely open to supporting more complete checkers, but my goal is to have something in tree and enabled by default. I worry that out of tree things lead to the maintenance burden being in the wrong place as developers commit changes that haven't been checked (at least until some future day when the project has more complete pre-commit CI).

My reference to abi-compliance-checker is to show how would our final solution for the ABI backward compatibility check look finally. I have no objections about smaller/partial checkers integrated into the build.

With that foreword, please note that I am not much versed in our make/build system.

[brooks]

I think I'm going to redo this using readelf. There is some annoying differences between the versions (binutil, elftoolchain, and llvm), but the format feels more manageable and I can see a path to checking for symbols changing type and weak vs strong.

[brooks]

• Rewrite with readelf and an awk script. Weak vs global and symbol type are now tracked. It's also a bit less grotty as readelf implementations are more similar.
• Make the symref files dot-files so they clutter the tree less.

[brooks]

typo fix

[sjg]

share/mk/bsd.lib.mk

379

It is conventional to have white-space after =
same applies to all below

380

Perhaps && !target(${SHLIB_NAME}.symbols)
so a local*mk could provide an alternative implementation

383

Why printf when echo would do?

392

what's wrong with .if make(check-symbols) ?

[kib]

Private versioning namespace should be filtered out, I think.

Our readelf adds something after the symbol+version, like

8: 00000000000bba00    17 FUNC    GLOBAL DEFAULT   13 fabs@@FBSD_1.0 (2)

Note the (2) part. It should be removed, if not already.

Perhaps the visibility column (Vis) should also participate in the final result.

tools/build/symlist.awk

50 ↗

(On Diff #137739)

Wouldn't -D switch for readelf do this?

[brooks]

Review suggestions from @sjg

[brooks]

In D44271#1025583, @kib wrote:

Private versioning namespace should be filtered out, I think.

I think we want to know when that set changes even if we don't consider it part of the ABI, but I probably could be persuaded otherwise. I might want to make it optional so libraries can opt in and others opt out.

Our readelf adds something after the symbol+version, like

8: 00000000000bba00    17 FUNC    GLOBAL DEFAULT   13 fabs@@FBSD_1.0 (2)

Note the (2) part. It should be removed, if not already.

awk parsing takes care of that.

Perhaps the visibility column (Vis) should also participate in the final result.

In theory I think so, but I've not found an example were it's anything other than DEFAULT so I'm not sure it adds value.

share/mk/bsd.lib.mk

379

I've added spaces as with earlier assignments in the file. Overall the file needs a sweep as there are spaces, tabs, and nothing...

380

I've added this guard, but I'm not quite sure it's the right one. The question is, what do we want to be pluggable?

383

This file needs to not contain the string @generated or tools like Phabricator will treat it as a generated file and not display diffs by default. printf accomplishes this.

tools/build/symlist.awk

50 ↗

(On Diff #137739)

Unfortunately llvm-readelf doesn't support -D today and I'd like to avoid the need to bootstrap readelf. We should probably add -D support, but I'm at the mercy of llvm15 toolchains downstream so it will be a long time before we can rely on it.

[kib]

In D44271#1026288, @brooks wrote:

In D44271#1025583, @kib wrote:

Private versioning namespace should be filtered out, I think.

I think we want to know when that set changes even if we don't consider it part of the ABI, but I probably could be persuaded otherwise. I might want to make it optional so libraries can opt in and others opt out.

At least it should be split into two parts somehow, either textually in the same file, or putting the FBSDprivate into aux file at all.

tools/build/symlist.awk

50 ↗

(On Diff #137739)

We have 18 in the tree?
At least please comment about the reason of using this check (which is also not guaranteed to work if sections are stripped).

[brooks]

Explain why we can't use readelf -D and add symbol visibility.

[brooks]

In D44271#1026327, @kib wrote:

In D44271#1026288, @brooks wrote:

In D44271#1025583, @kib wrote:

Private versioning namespace should be filtered out, I think.

I think we want to know when that set changes even if we don't consider it part of the ABI, but I probably could be persuaded otherwise. I might want to make it optional so libraries can opt in and others opt out.

At least it should be split into two parts somehow, either textually in the same file, or putting the FBSDprivate into aux file at all.

After further thought, enabling --no-undefined-version is probably enough to keep track of FBSDprivate so I'll remove them from the symref file.

[brooks]

Exclude private symbols

[brooks]

Only complain about missing symref file in the current directory.
Otherwise, it's too noisy in lib/libc due to libraries in the tests tree.

[kib]

share/mk/bsd.lib.mk

391

My last request: can we avoid using hidden file names for symrefs? Why should we hide them?

[brooks]

share/mk/bsd.lib.mk

391

It feels noisy to me to have them visible, particularly when they are MD, but I've waffled on this.

[kib]

share/mk/bsd.lib.mk

391

Put them into a subdir?

[sjg]

share/mk/bsd.lib.mk

383

You could do ?

echo ${:U@}generated

[brooks]

• allow local override of ${SHLIB_NAME}.symbols (requested by @sjg)
• don't hide symref files (requested by @kib)

[brooks]

Eliminate the use of printf

Adapt the path search logic from bsd.symver.mk to find the awk script.
This should be robust to out of tree bsd.lib.mk use so long as the awk
script is also installed.

[brooks]

actually install symref.awk

[brooks]

I noticed we mostly don't use printf in share/mk so switched to the ${:U@} trick. When doing that I noticed that bsd.symver.mk finds it's awk script with a path search so I switched to that model.

[sjg]

share/mk/bsd.lib.mk

386

fwiw this whole for loop can be replaced with
_mpath := ${.MAKEFLAGS:tW:S/-m /-m/g:tw:M-m*:S/-m//g}

ie. treat .MAKESYSFLAGS as one big string, lose the space after -m
turn it back into words, match only -m* and lose the -m

402

FWIW I would use := here