pmc: avoid illegal flexible array member

[rlibby]

Summary

Remove struct pmchdr_cpuidinfo which was just a wrapper around a
flexible array member of uint32_t. Flexible array members are
non-standard in C++, and even in C are not allowed as the only member
of a struct.

GCC errored out on pmchdr_cpuidinfo, but did not complain about
pmchdr_pmcinfo, so I left it alone here, though it is also non-standard.

Fixes: 93da997ef759 ("pmc: new pmc log processing framework")

Test Plan

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

[adrian]

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!

[ali_mashtizadeh.com]

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!

@adrian You asked me this in the review. CPUID is only for x86, because we will need to patch errata for IBS and eventually PEBS.
For other processors we currently don’t need much state and that’s just the cpuinfo.

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.

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.

[ali_mashtizadeh.com]

usr.sbin/pmc/headers.hh

74

You may as well change this to 0 to be consistent.

79

Just changing this to cpuid[0] or cpuid[1] would eliminate this error in my out of tree build. See the comment.

[rlibby]

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/

[ali_mashtizadeh.com]

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/

Thanks for the pointers! I didn't realize we were still building with gcc. Seems like they just don't like a single memory that has variable size.

I'll test my outstanding patches to make sure there's no other breakages and refresh them.