In D59355#1363179, @rlibby wrote:

In D59355#1363034, @ali_mashtizadeh.com wrote:

Yes it’s non-standard but we already depend on a number of gnu extensions in our code. I think is just an error with the build flags, if you just set the length to 0 or 1 and it’s also accepted by gcc.

I'm not sure why gcc accepts the flexible array member in pmchdr_pmcinfo and not the one in pmchdr_cpuidinfo. It seems it should allow them both as language extensions, or not. Zero-length arrays are slightly different from flexible array members in C, though both are illegal in standard C++. I guess the zero-length array avoids the size and alignment ambiguity.

Anyway, I tried your suggestion but gcc only accepted it for pmchdr_cpuidinfo::cpuid. It issued a fatal warning when it was applied to pmchdr_pmcinfo::pmc, -Werror=stringop-overread. I do not know why it doesn't issue the same fatal warning for pmchdr_cpuidinfo::cpuid.

My only interest here is getting the build working, so if you want to do non-standard C++ things, I am not going to object. I would say though that I do not understand the advantage of the non-standard way over the standard way. Why go through the bother of C++ just to work around it for something fairly trivial?

How do you build this with gcc so I can test my other patches? When I built it using my original repo I see a few more errors because of differences in our build flags.

I'll update the testing section, but in short, I do

sudo pkg install amd64-gcc14
env MAKEOBJDIRPREFIX=/usr/obj/gcc14 CROSS_TOOLCHAIN=amd64-gcc14 make buildworld

CI does essentially the same:
https://github.com/freebsd/freebsd-ci/blob/main/jobs/FreeBSD-main-amd64-gcc14_build/build.sh

And that job has been tripping since this went in:
https://ci.freebsd.org/job/FreeBSD-main-amd64-gcc14_build/4343/

In D59355#1363034, @ali_mashtizadeh.com wrote:

Yes it’s non-standard but we already depend on a number of gnu extensions in our code. I think is just an error with the build flags, if you just set the length to 0 or 1 and it’s also accepted by gcc.

In D59355#1361931, @adrian wrote:

oh i see what's going on here. ugh. :-) I'm ok with this, but gosh we need a less cpu specific way of doing all of this!

oh i see what's going on here. ugh. :-) I'm ok with this, but gosh we need a less cpu specific way of doing all of this!

oh good catch! (I've found/fixed these width bugs in the deep past, they can be a pain to find as not every counter is the damned same... :-) )

You are correct, thank you for the observation. pmcstat does not exercise amd_get_msr() directly. The correct test path is via pmc_x86_get_msr() from libpmc, which issues a PMC_OP_GETMSR ioctl into the kernel amd_get_msr() code path.
Hardware validation was performed on AMD Ryzen 5 5600X (Family 19h, Zen 3), FreeBSD 16.0-CURRENT (hwpmc-amd-work-n284229-d18be873e2c2):

The change seems correct to me, but the test plan does not; pmcstat does not make any use of the pmc_get_msr() code path. Perhaps you have some other use-case or test program that you used?

In D32316#741213, @emaste wrote:

@ray I assume you can go ahead with committing this one?

@ray I assume you can go ahead with committing this one?

In D32316#737883, @ray wrote:

In D32316#737876, @mhorne wrote:

It seems to me that only the changes to pmc_arm64_initialize() should be necessary, because it handles optional classes in the same way that pmc_intel_initialize() does, by passing the correct nclasses value to pmc_mdep_alloc().

Problem here is in static machdep class numbers. If classes will be initialized in incorrect order, adjusted ri will be incorrect. So that modification may save some time on debugging such issue for new optional classes with just little time in hwpmc(4) init.

In D32316#737876, @mhorne wrote:

It seems to me that only the changes to pmc_arm64_initialize() should be necessary, because it handles optional classes in the same way that pmc_intel_initialize() does, by passing the correct nclasses value to pmc_mdep_alloc().

It seems to me that only the changes to pmc_arm64_initialize() should be necessary, because it handles optional classes in the same way that pmc_intel_initialize() does, by passing the correct nclasses value to pmc_mdep_alloc().

Update classes base ri to handle optional classes out of order.

Comment update LGTM

Squash commits.

Fix comment.

In D30047#676281, @mhorne wrote:

This looks fine to me, thanks for fixing this.

It seems that pmc_id_t should be an opaque type for consumers of libpmc? I.e. the only userspace consumer of these bit macros should be libpmc itself.

If that is the case, then this change should be okay without a PMC_VERSION bump. There are no uses of PMC_ID_TO_MODE or PMC_ID_MAKE_ID in libpmc. However, I did not check if this was true for older releases.