Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 12/28/20 10:15 PM, Allan McRae wrote:
On 25/12/20 2:05 am, Emil Velikov wrote:
On Thu, 24 Dec 2020 at 01:38, Eli Schwartz wrote:
On 12/23/20 5:42 PM, Emil Velikov wrote:
All the required public API is annotated with SYMEXPORT, so we can just
add the meson notation, to hide all the symbols by default.
Thus we no longer spill all the internal API into the global namespace.
Thanks for noticing this... it's a regression from autotools, which
contained in lib/libalpm/Makefile.am:
if ENABLE_VISIBILITY_CC
if DARWIN
AM_CFLAGS += -fvisibility=hidden
else
AM_CFLAGS += -fvisibility=internal
endif
endif
I wonder if we had a good reason to use "internal" and if we should
continue to do so? IIUC it makes it slightly more optimized at the cost
of allowing pointers into private functions (e.g. callbacks) used by
other programs to segfault.
If the output of size is any indication - there is little-to-no
optimisation happening.
The former produces the same numbers, while the latter claims that
binaries built with "internal" are larger by 8 bytes (always).
Fwiw the above snippet is the first time I've seen anyone using
"internal", after staring at various projects for 5+ years.
Can bring it back, simply not sure it brings much.
Thanks
Emil
$ ls
812336 build-internal/libalpm.so.12.0.1
812328 build-hidden/libalpm.so.12.0.1
337176 build-internal/pacman
337168 build-hidden/pacman
$ size
3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1
3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1
15528850405808 166136 288f8 build-internal/pacman
15528850405808 166136 288f8 build-hidden/pacman
It turns out, we have this:
#define SYMHIDDEN __attribute__((visibility("internal")))
But we never flag any functions with this. That would enable a compiler
to optimize for speed and not compatibility, in which case using
-fvisibility=internal would make a difference.
I'm not sure if we removed any usage of that from the codebase, or if it
was never there...
Hmm... only ever used on alpm_add_target (then _alpm_add_loadtarget) and
removed in commit 7f7da2b5fc01f46d28236384540c7ecfdac16a63 which added
the AM_CFLAGS instead and marked a bunch of symbols as SYMEXPORT.
But that define used to be for visibility("hidden"), then in commit
920b0d2049deb148efe89bfebda03d172b68c1f5 both the (unused) define and
the AM_CFLAGS got switched to internal: "this allows for slightly better
optimization".
Which is about as vague as our knowledge today. "Supposedly better and
probably works fine, but no one put out numbers on it".
idk, "hidden" is probably just fine.
--
Eli Schwartz
Bug Wrangler and Trusted User
OpenPGP_signature
Description: OpenPGP digital signature
[pacman-dev] [GIT] The official pacman repository branch, master, updated. v6.0.0alpha1-26-ga023565e
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "The official pacman repository". The branch, master has been updated via a023565ed370851fd5bf61298460fe0adb0b4189 (commit) via ccdd1e3fd92591755e2b94bf63416c7b30cd217a (commit) via 831fc568fc87a75bb6e05575b93a7541b49e7aba (commit) from 95ffdd68b250af1d37067fe6dd70fc6a6094bc62 (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log - commit a023565ed370851fd5bf61298460fe0adb0b4189 Author: Eli Schwartz Date: Mon Dec 28 21:36:35 2020 -0500 doc: make doxygen build from any directory In the autotools build, it only built in-tree, from cwd = doc/ and resolving doc/../lib/libalpm In the meson build, this accidentally worked if cwd = pacman/builddir/ and resolved to builddir/../lib/libalpm/ But... this should always have been configured with the actual path to the inputs. So, we will now proceed to do so. Fixes building man3 if your out of tree builddir doesn't happen to be a direct subdirectory of the source root. Signed-off-by: Eli Schwartz Signed-off-by: Allan McRae commit ccdd1e3fd92591755e2b94bf63416c7b30cd217a Author: Emil Velikov Date: Wed Dec 23 22:43:57 2020 + Move hex_representation() to src/common We'll reuse the function in pacman with a later commit. Signed-off-by: Emil Velikov Signed-off-by: Allan McRae commit 831fc568fc87a75bb6e05575b93a7541b49e7aba Author: Emil Velikov Date: Wed Dec 23 22:43:56 2020 + Remove pre libarchive 3.0 code Pacman has required libarchive 3.0 or later for quite some time mow. Signed-off-by: Emil Velikov Signed-off-by: Allan McRae --- Summary of changes: doc/Doxyfile.in | 4 ++-- doc/meson.build | 1 + lib/libalpm/libarchive-compat.h | 20 lib/libalpm/util.c | 24 src/common/util-common.c| 26 ++ src/common/util-common.h| 1 + 6 files changed, 30 insertions(+), 46 deletions(-) hooks/post-receive -- The official pacman repository
Re: [pacman-dev] [PATCH 3/3] meson: use hidden symbol visiblity by default
On 25/12/20 2:05 am, Emil Velikov wrote:
> On Thu, 24 Dec 2020 at 01:38, Eli Schwartz wrote:
>>
>> On 12/23/20 5:42 PM, Emil Velikov wrote:
>>> All the required public API is annotated with SYMEXPORT, so we can just
>>> add the meson notation, to hide all the symbols by default.
>>>
>>> Thus we no longer spill all the internal API into the global namespace.
>> Thanks for noticing this... it's a regression from autotools, which
>> contained in lib/libalpm/Makefile.am:
>>
>> if ENABLE_VISIBILITY_CC
>> if DARWIN
>> AM_CFLAGS += -fvisibility=hidden
>> else
>> AM_CFLAGS += -fvisibility=internal
>> endif
>> endif
>>
>>
>> I wonder if we had a good reason to use "internal" and if we should
>> continue to do so? IIUC it makes it slightly more optimized at the cost
>> of allowing pointers into private functions (e.g. callbacks) used by
>> other programs to segfault.
>>
> If the output of size is any indication - there is little-to-no
> optimisation happening.
> The former produces the same numbers, while the latter claims that
> binaries built with "internal" are larger by 8 bytes (always).
>
> Fwiw the above snippet is the first time I've seen anyone using
> "internal", after staring at various projects for 5+ years.
> Can bring it back, simply not sure it brings much.
>
> Thanks
> Emil
>
> $ ls
> 812336 build-internal/libalpm.so.12.0.1
> 812328 build-hidden/libalpm.so.12.0.1
> 337176 build-internal/pacman
> 337168 build-hidden/pacman
>
> $ size
> 3167083080 592 320380 4e37c build-internal/libalpm.so.12.0.1
> 3167083080 592 320380 4e37c build-hidden/libalpm.so.12.0.1
> 15528850405808 166136 288f8 build-internal/pacman
> 15528850405808 166136 288f8 build-hidden/pacman
>
It turns out, we have this:
#define SYMHIDDEN __attribute__((visibility("internal")))
But we never flag any functions with this. That would enable a compiler
to optimize for speed and not compatibility, in which case using
-fvisibility=internal would make a difference.
I'm not sure if we removed any usage of that from the codebase, or if it
was never there...
Allan
Re: [pacman-dev] [PATCH 3/3] pacman: add file checksum validation against mtree
On 24/12/20 8:43 am, Emil Velikov wrote:
> With libarchive v3.5.0 we have API to fetch the digest from the mtree.
> Use that to validate if the installed files are modified or not.
>
> As always, a modified backup file will trigger a warning but will not
> result in an actual failure.
>
> TODO: localization... no idea how that is even done :-)
Adding the _() around the strings is all that needed done.
> NOTE: indentation is likely all over the place - first time I see ts=2
For line wraps, we generally just use two indents, so really does not
matter what the tab system is.
> Signed-off-by: Emil Velikov
> ---
> src/pacman/check.c | 66 +-
> 1 file changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/src/pacman/check.c b/src/pacman/check.c
> index 02217d0f..083c547d 100644
> --- a/src/pacman/check.c
> +++ b/src/pacman/check.c
> @@ -176,19 +176,70 @@ static int check_file_size(const char *pkgname, const
> char *filepath,
> return 0;
> }
>
> -/* placeholders - libarchive currently does not read checksums from mtree
> files
> -static int check_file_md5sum(const char *pkgname, const char *filepath,
> - struct stat *st, struct archive_entry *entry, int backup)
> +#if ARCHIVE_VERSION_NUMBER >= 3005000
This does not need wrapped in #if. There is nothing libarchive specific
in this function.
> +static int check_file_cksum(const char *pkgname, const char *filepath,
> + int backup, const char *cksum_name, const char *cksum_calc,
> const char *cksum_mtree)
> {
> + if(!cksum_calc || !cksum_mtree) {
Only one of these failures matches the error message. Split into
"checkusm information not available" and "failed to calculate checksum"
> + if(!config->quiet) {
> + pm_printf(ALPM_LOG_WARNING, _("%s: %s (failed to
> compute %s checksum)\n"),
> + pkgname, filepath, cksum_name);
> + }
> + return 1;
> + }
> +
> + if(strcmp(cksum_calc, cksum_mtree) != 0) {
> + if(backup) {
> + if(!config->quiet) {
> + printf("%s%s%s: ", config->colstr.title,
> _("backup file"),
> + config->colstr.nocolor);
> + printf(_("%s: %s (%s checksum mismatch)\n"),
> + pkgname, filepath, cksum_name);
> + }
> + return 0;
> + }
> + if(!config->quiet) {
> + pm_printf(ALPM_LOG_WARNING, _("%s: %s (%s checksum
> mismatch)\n"),
> + pkgname, filepath, cksum_name);
> + }
> + return 1;
OK.
> + }
> +
> return 0;
> }
> +#endif
> +
> +static int check_file_md5sum(const char *pkgname, const char *filepath,
> + struct archive_entry *entry, int backup)
> +{
> + int errors = 0;
> +#if ARCHIVE_VERSION_NUMBER >= 3005000
> + char *cksum_calc = alpm_compute_md5sum(filepath);
> + char *cksum_mtree = hex_representation(archive_entry_digest(entry,
> +
> ARCHIVE_ENTRY_DIGEST_MD5), 16);
> + errors = check_file_cksum(pkgname, filepath, backup, "MD5", cksum_calc,
> +
> cksum_mtree);
> + free(cksum_mtree);
> + free(cksum_calc);
> +#endif
> + return (errors != 0 ? 1 : 0);
> +}
>
> static int check_file_sha256sum(const char *pkgname, const char *filepath,
> - struct stat *st, struct archive_entry *entry, int backup)
> + struct archive_entry *entry, int backup)
> {
> - return 0;
> + int errors = 0;
> +#if ARCHIVE_VERSION_NUMBER >= 3005000
> + char *cksum_calc = alpm_compute_sha256sum(filepath);
> + char *cksum_mtree = hex_representation(archive_entry_digest(entry,
> +
> ARCHIVE_ENTRY_DIGEST_SHA256), 32);
> + errors = check_file_cksum(pkgname, filepath, backup, "SHA256",
> cksum_calc,
> +
> cksum_mtree);
> + free(cksum_mtree);
> + free(cksum_calc);
> +#endif
> + return (errors != 0 ? 1 : 0);
> }
> -*/
>
> /* Loop through the files of the package to check if they exist. */
> int check_pkg_fast(alpm_pkg_t *pkg)
> @@ -369,7 +420,8 @@ int check_pkg_full(alpm_pkg_t *pkg)
>
> if(type == AE_IFREG) {
> file_errors += check_file_size(pkgname, filepath, ,
> entry, backup);
> - /* file_errors += check_file_md5sum(pkgname, filepath,
> , entry, backup); */
> + file_errors += check_file_md5sum(pkgname, filepath,
> entry, backup);
> +
Re: [pacman-dev] [PATCH 1/3] Remove pre libarchive 3.0 code
On 24/12/20 8:43 am, Emil Velikov wrote: > Pacman has required libarchive 3.0 or later for quite some time mow. > > Signed-off-by: Emil Velikov > --- > lib/libalpm/libarchive-compat.h | 20 > 1 file changed, 20 deletions(-) > Looks good.
[pacman-dev] [PATCH] doc: make doxygen build from any directory
In the autotools build, it only built in-tree, from cwd = doc/ and
resolving doc/../lib/libalpm
In the meson build, this accidentally worked if cwd =
pacman/builddir/ and resolved to builddir/../lib/libalpm/
But... this should always have been configured with the actual path to
the inputs. So, we will now proceed to do so.
Fixes building man3 if your out of tree builddir doesn't happen to be a
direct subdirectory of the source root.
Signed-off-by: Eli Schwartz
---
doc/Doxyfile.in | 4 ++--
doc/meson.build | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/doc/Doxyfile.in b/doc/Doxyfile.in
index 776318da7..6744e7659 100644
--- a/doc/Doxyfile.in
+++ b/doc/Doxyfile.in
@@ -117,8 +117,8 @@ WARN_LOGFILE =
#---
# Configuration options related to the input files
#---
-INPUT = ../lib/libalpm/alpm.h \
- ../lib/libalpm/alpm_list.h
+INPUT = @INPUT_DIRECTORY@/../lib/libalpm/alpm.h \
+ @INPUT_DIRECTORY@/../lib/libalpm/alpm_list.h
INPUT_ENCODING = UTF-8
FILE_PATTERNS =
RECURSIVE = NO
diff --git a/doc/meson.build b/doc/meson.build
index 570dc7654..4aaac5540 100644
--- a/doc/meson.build
+++ b/doc/meson.build
@@ -135,6 +135,7 @@ meson.add_install_script(MESON_MAKE_SYMLINK,
doxygen = find_program('doxygen', required : get_option('doxygen'))
if doxygen.found() and not get_option('doxygen').disabled()
doxyconf = configuration_data()
+ doxyconf.set('INPUT_DIRECTORY', meson.current_source_dir())
doxyconf.set('OUTPUT_DIRECTORY', meson.current_build_dir())
doxyfile = configure_file(
input : 'Doxyfile.in',
--
2.29.2
