Message ID | 20200830100058.4180798-1-fontaine.fabrice@gmail.com |
---|---|
State | Rejected |
Headers | show |
Series | [PATCH/next,1/2] package/libcap: drop host-gperf dependency | expand |
Hello Fabrice, On Sun, 30 Aug 2020 12:00:57 +0200 Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > host-gperf dependency was added in commit > 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the > commit message but gperf can be disabled through BUILD_GPERF since > https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba > > So use this variable and drop this unneeded dependency > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Thanks for looking into this. I agree that BUILD_GPERF=no will avoid the host-gperf dependency, however I'm a bit confused how this works in libcap and whether it has any drawback. When gperf is detected, it will be used to generate some C code that contains a hash function: ifeq ($(BUILD_GPERF),yes) USE_GPERF_OUTPUT = $(GPERF_OUTPUT) INCLUDE_GPERF_OUTPUT = -DINCLUDE_GPERF_OUTPUT='"$(GPERF_OUTPUT)"' endif $(GPERF_OUTPUT): cap_names.list.h perl -e 'print "struct __cap_token_s { const char *name; int index; };\n%{\nconst struct __cap_token_s *__cap_lookup_name(const char *, size_t);\n%}\n%%\n"; while ($$l = <>) { $$l =~ s/[\{\"]//g; $$l =~ s/\}.*// ; print $$l; }' < $< | gperf --ignore-case --language=ANSI-C --readonly --null-strings --global-table --hash-function-name=__cap_hash_name --lookup-function-name="__cap_lookup_name" -c -t -m20 $(INDENT) > $@ sed -e 's/unsigned int len/size_t len/' -i $@ cap_text.o: cap_text.c $(USE_GPERF_OUTPUT) $(INCLS) $(CC) $(CFLAGS) $(IPATH) $(INCLUDE_GPERF_OUTPUT) -c $< -o $@ Then in cap_text.c, this gets used like this: #ifdef INCLUDE_GPERF_OUTPUT /* we need to include it after #define _GNU_SOURCE is set */ #include INCLUDE_GPERF_OUTPUT #endif So the gperf-generated source file, if it exists is included. But I don't see any conditional code that makes use of it when available. Am I missing something? I wanted to understand the impact of not having gperf. Perhaps not having a hash function makes libcap significantly slower ? Initially, I was expected libcap to have a pre-generated version of the file, and having gperf available would only allow to re-generate the file, but that doesn't seem to be what's happening. Do you understand a bit better what is going on here ? Thanks, Thomas
Hello Thomas, Le jeu. 3 sept. 2020 à 22:47, Thomas Petazzoni <thomas.petazzoni@bootlin.com> a écrit : > > Hello Fabrice, > > On Sun, 30 Aug 2020 12:00:57 +0200 > Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > > > host-gperf dependency was added in commit > > 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the > > commit message but gperf can be disabled through BUILD_GPERF since > > https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba > > > > So use this variable and drop this unneeded dependency > > > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > > Thanks for looking into this. I agree that BUILD_GPERF=no will avoid > the host-gperf dependency, however I'm a bit confused how this works in > libcap and whether it has any drawback. > > When gperf is detected, it will be used to generate some C code that > contains a hash function: > > ifeq ($(BUILD_GPERF),yes) > USE_GPERF_OUTPUT = $(GPERF_OUTPUT) > INCLUDE_GPERF_OUTPUT = -DINCLUDE_GPERF_OUTPUT='"$(GPERF_OUTPUT)"' > endif > > $(GPERF_OUTPUT): cap_names.list.h > perl -e 'print "struct __cap_token_s { const char *name; int index; };\n%{\nconst struct __cap_token_s *__cap_lookup_name(const char *, size_t);\n%}\n%%\n"; while ($$l = <>) { $$l =~ s/[\{\"]//g; $$l =~ s/\}.*// ; print $$l; }' < $< | gperf --ignore-case --language=ANSI-C --readonly --null-strings --global-table --hash-function-name=__cap_hash_name --lookup-function-name="__cap_lookup_name" -c -t -m20 $(INDENT) > $@ > sed -e 's/unsigned int len/size_t len/' -i $@ > > cap_text.o: cap_text.c $(USE_GPERF_OUTPUT) $(INCLS) > $(CC) $(CFLAGS) $(IPATH) $(INCLUDE_GPERF_OUTPUT) -c $< -o $@ > > Then in cap_text.c, this gets used like this: > > #ifdef INCLUDE_GPERF_OUTPUT > /* we need to include it after #define _GNU_SOURCE is set */ > #include INCLUDE_GPERF_OUTPUT > #endif > > So the gperf-generated source file, if it exists is included. But I > don't see any conditional code that makes use of it when available. gperf code has been added with https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=e57378c88b6144ff9c06777ff0e0c9d722eeefd3 From my understanding, gperf code is used in libcap/cap_text.c by the lookupname function: #ifdef GPERF_DOWNCASE const struct __cap_token_s *token_info; int c; unsigned len; for (len=0; (c = str.constp[len]); ++len) { if (!(isalpha(c) || (c == '_'))) { break; } } token_info = __cap_lookup_name(str.constp, len); if (token_info != NULL) { *strp = str.constp + len; return token_info->index; } #else /* ie., ndef GPERF_DOWNCASE */ lookupname is used by cap_from_name which is a part of the public API and is also used by capsh program. > > Am I missing something? I wanted to understand the impact of not having > gperf. Perhaps not having a hash function makes libcap significantly > slower ? Yes, you're right, not having gperf will have an unknown (but probably small?) performance impact. I didn't take time to measure it on an embedded target but we should probably keep building host-gperf for the target (and probably also for the host as host-gperf is not a "huge" dependency). > > Initially, I was expected libcap to have a pre-generated version of the > file, and having gperf available would only allow to re-generate the > file, but that doesn't seem to be what's happening. > > Do you understand a bit better what is going on here ? > > Thanks, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Best Regards, Fabrice
On Sun, 30 Aug 2020 12:00:57 +0200 Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > host-gperf dependency was added in commit > 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the > commit message but gperf can be disabled through BUILD_GPERF since > https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba > > So use this variable and drop this unneeded dependency > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> > --- > package/libcap/libcap.mk | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) So we discussed this with Yann, Peter and Arnout and found that host-gperf is really quick to build, and provides a better way for libcap to lookup symbols, thanks to a hash table. So we prefer to keep things as they are with the host-gperf dependency. Thomas
diff --git a/package/libcap/libcap.mk b/package/libcap/libcap.mk index 08c1bc9deb..208f8eade1 100644 --- a/package/libcap/libcap.mk +++ b/package/libcap/libcap.mk @@ -10,11 +10,9 @@ LIBCAP_SOURCE = libcap-$(LIBCAP_VERSION).tar.xz LIBCAP_LICENSE = GPL-2.0 or BSD-3-Clause LIBCAP_LICENSE_FILES = License -LIBCAP_DEPENDENCIES = host-libcap host-gperf +LIBCAP_DEPENDENCIES = host-libcap LIBCAP_INSTALL_STAGING = YES -HOST_LIBCAP_DEPENDENCIES = host-gperf - ifeq ($(BR2_STATIC_LIBS),y) LIBCAP_MAKE_TARGET = libcap.a libcap.pc LIBCAP_MAKE_INSTALL_TARGET = install-static @@ -28,7 +26,8 @@ endif LIBCAP_MAKE_FLAGS = \ BUILD_CC="$(HOSTCC)" \ - BUILD_CFLAGS="$(HOST_CFLAGS)" + BUILD_CFLAGS="$(HOST_CFLAGS)" \ + BUILD_GPERF=no ifeq ($(BR2_PACKAGE_LIBCAP_TOOLS),y) define LIBCAP_BUILD_TOOLS_CMDS @@ -62,12 +61,12 @@ endef define HOST_LIBCAP_BUILD_CMDS $(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) -C $(@D)\ - RAISE_SETFCAP=no + BUILD_GPERF=no RAISE_SETFCAP=no endef define HOST_LIBCAP_INSTALL_CMDS $(HOST_MAKE_ENV) $(MAKE) -C $(@D) prefix=$(HOST_DIR) \ - RAISE_SETFCAP=no lib=lib install + BUILD_GPERF=no RAISE_SETFCAP=no lib=lib install endef $(eval $(generic-package))
host-gperf dependency was added in commit 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the commit message but gperf can be disabled through BUILD_GPERF since https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba So use this variable and drop this unneeded dependency Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> --- package/libcap/libcap.mk | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)