Details

Reviewers

Commits

rS346196: Fix warnings with lib/libpmc

Summary

Use MIN instead of similar hand rolled macro.
Sort headers.
Use errno.h instead of sys/errno.h.
Wrap sizeof argument in parentheses.
Remove __BSD_VISIBLE and _XOPEN_SOURCE #defines to mute warnings about incompatible snprintf definitions.

This fixes a number of warnings I've been seeing lately in my builds.

Sort makefile variables per style.Makefile(9) (CFLAGS/CWARNFLAG.gcc) and bump WARNS to 3.

MFC after: 2 weeks

Test Plan

make tinderbox

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

LGTM.

lib/libpmc/pmu-events/jevents.h
14 ↗	(On Diff #55970)	This `min()` macro is a lot better than the one from sys/param.h since it only evaluates arguments once and checks that they are of the same type. Shouldn't matter here since I'm sure clang is smart enough to not evaluate `(unsigned)rlim.rlim_max / 2` twice. I would really like a sensible min()+max() macro but I'm not sure we can change MIN/MAX since there might be some (probably broken) uses that rely on arguments being evaluated twice.

lib/libpmc/pmu-events/jevents.h
14 ↗	(On Diff #55970)	The problem is that it was triggering `-Wconversion` issues because one of the values was interpreted as unsigned pointer and the other as signed pointer (!?). Furthermore, `typeof` is a non-standard GNU extension: https://stackoverflow.com/a/12081525, https://en.wikipedia.org/wiki/Typeof . I don't think we should be relying on GNU extensions like this.

Add forgotten Makefile.inc

lib/libpmc/pmu-events/jevents.h
14 ↗	(On Diff #55970)	`typeof` (or `__typeof`/ `__typeof__`) is already used in quite a few places in the source tree (e.g. in `__containerof`) so I'm pretty sure it isn't possible to build with a compiler that doesn't support typeof. To fix the -Wconversion, I think changing the 512 to 512u should also work. I think removing yet another custom min/max macro is good, I'm just wondering if we can improve the default one from param.h.

ngie marked an inline comment as done.Apr 10 2019, 5:22 AM

ngie added inline comments.

lib/libpmc/pmu-events/jevents.h
14 ↗	(On Diff #55970)	I'd be game to improve the existing macro... it's just that we'd need to be careful/judicious about it, since it would affect a number of downstream consumers of the macro (both internal and external to the base system). At the end of the day, would it really be worth the effort for something that's been in place for many moons? About the proposed -Wconversion fix, that would work, unless the values compared are variable depending on the architecture. I recently embarked on this quest for the Google Test project, and it proved annoying to say the least: https://github.com/google/googletest/pull/2170 .

lib/libpmc/pmu-events/jevents.c
867 ↗	(On Diff #55974)	Casting `rlim.rlim_max` to unsigned is bogus :/.

Remove bogus (unsigned) cast with call to MIN().

lib/libpmc/Makefile
107 ↗	(On Diff #56035)	I thought it was typical to have WARNS near the top, but looking now I see a variety of placements.
lib/libpmc/libpmc_json.cc
44–46 ↗	(On Diff #56035)	Why don't these sort with the rest?
lib/libpmc/pmu-events/jevents.c
4–5 ↗	(On Diff #56035)	Is there some sort of upstream for this?

ngie marked 3 inline comments as done.Apr 13 2019, 6:07 AM

ngie added inline comments.

lib/libpmc/Makefile
107 ↗	(On Diff #56035)	From `style.Makefile`: • Special variables (i.e., LIB, SRCS, MLINKS, etc.) are listed in order of “product”, then building and installing a binary. Special variables may also be listed in “build” order: i.e., ones for the primary program (or library) first. The general “product” order is: PROG/[SH]LIB/SCRIPTS FILES LINKS [NO_]MAN MLINKS INCS SRCS WARNS CFLAGS DPADD LDADD. The general “build” order is: PROG/[SH]LIB/SCRIPTS SRCS WARNS CFLAGS DPADD LDADD INCS FILES LINKS [NO_]MAN MLINKS. So I should move it up.
lib/libpmc/libpmc_json.cc
44–46 ↗	(On Diff #56035)	These aren't system headers and rely on other headers being included and other definitions be defined before them.
lib/libpmc/pmu-events/jevents.c
4–5 ↗	(On Diff #56035)	Hmm... @mmacy added this copyright attribute in rS334128 last year, so I think this is copy-pasted from another place. does a google search Wait.. it _was_ copied from one of the Intel folks' (andikleen@) repositories who works on Linux: https://github.com/andikleen/pmu-tools/blob/master/jevents/jevents.c . Why wasn't this noted in the original code import :(?

ngie marked 3 inline comments as done.Apr 13 2019, 6:20 AM

ngie added inline comments.

lib/libpmc/libpmc_json.cc
44–46 ↗	(On Diff #56035)	Ah, part of the problem is that some of the changes needed to make the libraries compile with C++ haven't been imported. In particular, we need to sprinkle `__BEGIN_DECLS`/`__END_DECLS` around the headers.