Paths

Table of Contentst

-
sys/cddl/dev/kinst/amd64/
-
cddl/
-
dev/
-
kinst/
-
amd64/
-
kinst_isa.c

Differential D40283

kinst: check for 'push %rbp' anywhere in the function
ClosedPublic
Actions

Authored by christos on May 26 2023, 1:44 PM.

Details

Reviewers

markj

Commits

rGea89133dbc5f: kinst: check for 'push %rbp' anywhere in the function

Summary

Currently kinst checks if only the first instruction is 'push %rbp',
essentially excluding functions that do push RBP, but not in the first
instruction.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 51708
Build 48599: arc lint + arc unit

Event Timeline

christos created this revision.May 26 2023, 1:44 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 26 2023, 1:44 PM

christos requested review of this revision.May 26 2023, 1:44 PM

Harbormaster completed remote builds in B51708: Diff 122470.May 26 2023, 1:44 PM

christos added a parent revision: D39229: kinst: add kinst_excluded().May 26 2023, 1:44 PM

christos removed a parent revision: D39229: kinst: add kinst_excluded().May 26 2023, 2:53 PM

Check for 'pop %rbp' as well.

Harbormaster completed remote builds in B51773: Diff 122608.May 30 2023, 2:07 PM

markj added inline comments.Jul 4 2023, 4:06 PM

sys/cddl/dev/kinst/amd64/kinst_isa.c
515–516	The first sentence isn't really accurate anymore. I'd just replace the comment with something like, "refuse to instrument functions lacking the usual frame pointer manipulations since they might correspond to exception handlers."

Address Mark's comment.

Harbormaster completed remote builds in B52668: Diff 124744.Jul 16 2023, 4:33 PM

Please note in the commit message that this behaviour matches that of FBT. That is, FBT will put an entry probe on any function that contains a push %rbp.

This revision is now accepted and ready to land.Jul 19 2023, 1:56 PM

Closed by commit rGea89133dbc5f: kinst: check for 'push %rbp' anywhere in the function (authored by christos). · Explain WhyJul 19 2023, 3:00 PM

This revision was automatically updated to reflect the committed changes.

christos added a commit: rGea89133dbc5f: kinst: check for 'push %rbp' anywhere in the function.

Revision Contents
Changeset List

Path

Size

sys/

cddl/

dev/

kinst/

amd64/

kinst_isa.c

17 lines

Diff 122470

View Options

sys/cddl/dev/kinst/amd64/kinst_isa.c

Show All 40 Lines
*		*
* CRC32 code derived from work by Gary S. Brown.		* CRC32 code derived from work by Gary S. Brown.
*/		*/

#include <sys/cdefs.h>		#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");		__FBSDID("$FreeBSD$");

#include <sys/param.h>		#include <sys/param.h>
		#include <sys/libkern.h>
#include <sys/systm.h>		#include <sys/systm.h>

		#if defined(__amd64__) \|\| defined(__i386__)
		#include <machine/md_var.h>
		#include <machine/specialreg.h>
		#endif

const uint32_t crc32_tab[] = {		const uint32_t crc32_tab[] = {
0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,		0x00000000, 0x77073096, 0xee0e612c, 0x990951ba, 0x076dc419, 0x706af48f,
0xe963a535, 0x9e6495a3, 0x0edb8832, 0x79dcb8a4, 0xe0d5e91e, 0x97d2d988,		0xe963a535, 0x9e6495a3, 0x0edb8832, 0x79dcb8a4, 0xe0d5e91e, 0x97d2d988,
0x09b64c2b, 0x7eb17cbd, 0xe7b82d07, 0x90bf1d91, 0x1db71064, 0x6ab020f2,		0x09b64c2b, 0x7eb17cbd, 0xe7b82d07, 0x90bf1d91, 0x1db71064, 0x6ab020f2,
0xf3b97148, 0x84be41de, 0x1adad47d, 0x6ddde4eb, 0xf4d4b551, 0x83d385c7,		0xf3b97148, 0x84be41de, 0x1adad47d, 0x6ddde4eb, 0xf4d4b551, 0x83d385c7,
0x136c9856, 0x646ba8c0, 0xfd62f97a, 0x8a65c9ec, 0x14015c4f, 0x63066cd9,		0x136c9856, 0x646ba8c0, 0xfd62f97a, 0x8a65c9ec, 0x14015c4f, 0x63066cd9,
0xfa0f3d63, 0x8d080df5, 0x3b6e20c8, 0x4c69105e, 0xd56041e4, 0xa2677172,		0xfa0f3d63, 0x8d080df5, 0x3b6e20c8, 0x4c69105e, 0xd56041e4, 0xa2677172,
0x3c03e4d1, 0x4b04d447, 0xd20d85fd, 0xa50ab56b, 0x35b5a8fa, 0x42b2986c,		0x3c03e4d1, 0x4b04d447, 0xd20d85fd, 0xa50ab56b, 0x35b5a8fa, 0x42b2986c,
▲ Show 20 Lines • Show All 685 Lines • ▼ Show 20 Lines	multitable_crc32c(uint32_t crc32c,
return (crc32c_sb8_64_bit(crc32c, buffer, length, to_even_word));		return (crc32c_sb8_64_bit(crc32c, buffer, length, to_even_word));
}		}

uint32_t		uint32_t
calculate_crc32c(uint32_t crc32c,		calculate_crc32c(uint32_t crc32c,
const unsigned char *buffer,		const unsigned char *buffer,
unsigned int length)		unsigned int length)
{		{
if (length < 4) {		if (length < 4) {
		jhbUnsubmitted Not Done Inline Actions You can just use `#ifdef __SSE4_2__`. -msse42 (or -msse4) will define that via the compiler driver. That is what controls 'popcnt' (in the case of popcnt we use it if you set CPUTYPE in your make.conf to specify a CPU that includes popcnt). jhb: You can just use `#ifdef __SSE4_2__`. -msse42 (or -msse4) will define that via the compiler…
		cemAuthorUnsubmitted Not Done Inline Actions Is `-msse4` used to compile this file on i386/amd64? I thought it was off by default, and only enabled on a few files. My build logs actually show `-mno-sse -mno-avx`. cem: Is `-msse4` used to compile this file on i386/amd64? I thought it was off by default, and only…
		markjUnsubmitted Not Done Inline Actions The entries in sys/conf/files.* explicitly specify -msse4. markj: The entries in sys/conf/files.* explicitly specify -msse4.
		cemAuthorUnsubmitted Not Done Inline Actions Only for libkern/x86/crc32_sse42.c. Not libkern/crc32.c (this file). cem: Only for libkern/x86/crc32_sse42.c. Not libkern/crc32.c (this file).
return (singletable_crc32c(crc32c, buffer, length));		return (singletable_crc32c(crc32c, buffer, length));
		#if defined(__amd64__) \|\| defined(__i386__)
		kibUnsubmitted Not Done Inline Actions Do we need defined(_KERNEL) there as well ? Why PC98 is excluded (it seems to be on the way out) ? kib: Do we need defined(_KERNEL) there as well ? Why PC98 is excluded (it seems to be on the way…
		cemAuthorUnsubmitted Not Done Inline Actions Is this code built outside of _KERNEL? tinderbox succeeds, so I don't believe this code is used outside of the kernel. PC98 is excluded because I believe it predates SSE4.2, and I preferred to avoid including dead SSE4.2 code in the PC98 kernel if it would never be used. cem: Is this code built outside of _KERNEL? tinderbox succeeds, so I don't believe this code is…
		} else if ((cpu_feature2 & CPUID2_SSE42) != 0) {
		return (sse42_crc32c(crc32c, buffer, length));
		#endif
} else {		} else {
return (multitable_crc32c(crc32c, buffer, length));		return (multitable_crc32c(crc32c, buffer, length));
}		}
}		}

kinst: check for 'push %rbp' anywhere in the functionClosedPublicActions