[v2] strip: Use debugedit instead of AWK to parse source files

Message ID 20220102172159.2902783-1-foxboron@archlinux.org
State New, archived
Headers show
Series [v2] strip: Use debugedit instead of AWK to parse source files | expand

Commit Message

Morten Linderud Jan. 2, 2022, 5:21 p.m. UTC
From: Morten Linderud <morten@linderud.pw>

This moves us from the fairly ugly AWK parsing line to debugedit which
originally comes out of the rpm project.

The original code has issues parsing anything that was not straight
C/C++ and languages like Rust or Go would return invalid source code
files. debugedit handles all these cases better.

Fixes FS#66755
Fixes FS#66888

Signed-off-by: Morten Linderud <morten@linderud.pw>
---
 scripts/libmakepkg/executable/debugedit.sh.in | 38 +++++++++++++++++++
 scripts/libmakepkg/tidy/strip.sh.in           | 25 +++++++++---
 2 files changed, 58 insertions(+), 5 deletions(-)
 create mode 100644 scripts/libmakepkg/executable/debugedit.sh.in

Comments

Allan McRae Jan. 2, 2022, 11:50 p.m. UTC | #1
On 3/1/22 03:21, Morten Linderud wrote:
> From: Morten Linderud <morten@linderud.pw>
> 
> This moves us from the fairly ugly AWK parsing line to debugedit which
> originally comes out of the rpm project.
> 
> The original code has issues parsing anything that was not straight
> C/C++ and languages like Rust or Go would return invalid source code
> files. debugedit handles all these cases better.
> 
> Fixes FS#66755
> Fixes FS#66888

Fixes FS#65677

I think that is all of them...

> Signed-off-by: Morten Linderud <morten@linderud.pw>

I'm going to make some opinionated changes to this patch.  No need to 
resubmit, but do let me know if the changes below are fine.

> ---
>   scripts/libmakepkg/executable/debugedit.sh.in | 38 +++++++++++++++++++
>   scripts/libmakepkg/tidy/strip.sh.in           | 25 +++++++++---
>   2 files changed, 58 insertions(+), 5 deletions(-)
>   create mode 100644 scripts/libmakepkg/executable/debugedit.sh.in
> 
> diff --git a/scripts/libmakepkg/executable/debugedit.sh.in b/scripts/libmakepkg/executable/debugedit.sh.in
> new file mode 100644
> index 00000000..ec0ab814
> --- /dev/null
> +++ b/scripts/libmakepkg/executable/debugedit.sh.in
> @@ -0,0 +1,38 @@
> +#!/usr/bin/bash
> +#
> +#   debugedit.sh - Confirm presence of debugedit binary
> +#
> +#   Copyright (c) 2011-2022 Pacman Development Team <pacman-dev@lists.archlinux.org>

Copyright (c) 2022

> +#
> +#   This program is free software; you can redistribute it and/or modify
> +#   it under the terms of the GNU General Public License as published by
> +#   the Free Software Foundation; either version 2 of the License, or
> +#   (at your option) any later version.
> +#
> +#   This program is distributed in the hope that it will be useful,
> +#   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#   GNU General Public License for more details.
> +#
> +#   You should have received a copy of the GNU General Public License
> +#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +[[ -n "$LIBMAKEPKG_EXECUTABLE_DEBUGEDIT_SH" ]] && return
> +LIBMAKEPKG_EXECUTABLE_DEBUGEDIT_SH=1
> +
> +LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
> +
> +source "$LIBRARY/util/message.sh"
> +source "$LIBRARY/util/option.sh"
> +
> +executable_functions+=('executable_debugedit')
> +
> +executable_debugedit() {
> +	if check_option "debug" "y"; then
> +		if ! type -p debugedit >/dev/null; then
> +			error "$(gettext "Cannot find the %s binary required for source files in debug packages.")" "debugedit"

required for including source files

> +			return 1
> +		fi
> +	fi
> +}
> diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in
> index 92a6fb15..84732c6f 100644
> --- a/scripts/libmakepkg/tidy/strip.sh.in
> +++ b/scripts/libmakepkg/tidy/strip.sh.in
> @@ -36,8 +36,23 @@ build_id() {
>   }
>   
>   source_files() {
> -	LANG=C readelf "$1" --debug-dump 2>/dev/null | \
> -		awk '/DW_AT_name +:/{name=$NF}/DW_AT_comp_dir +:/{{if (name == "<artificial>") next}{if (name !~ /^[<\/]/) {printf "%s/", $NF}}{print name}}'
> +	# debugedit rewrites the binary. In most cases this is handled by the gcc
> +	# prefix-map switches which rewrites $srcdir to $dbgsrcdir. debugedit will
> +	# not do anything in those cases. However for binaries that does not have a
> +	# prefix-map debugedit is going to modify the binary correcting the paths.
> +	#
> +	# From the mailing list discussion:
> +	# * Without -b/-d: it just lists all source files.
> +	# * With -b, it prints all source files rooted in the specified directory, but strips the
> +	#   prefix from the output.
> +	# * With both -b and -d, it replaces any base-dir prefixes with dest-dir
> +	#   (modifying the binary), then prints all paths rooted in the dest-dir
> +	#   (again, stripping the dest-dir prefix in the output).

I found this overkill, and not exactly clear.  I'm suggesting replacing 
with:

# This function does two things:
#
# 1) rewrites source file locations for packages not respecting prefix-
#    map switches.  This ensures all source file references in debug
#    info point to $dbgsrcdir.
#
# 2) outputs a list of files from the package source files to stdout
#    while stripping the $dbgsrcdir prefix

> +	LANG=C debugedit --no-recompute-build-id \
> +		--base-dir "${srcdir}" \
> +		--dest-dir "${dbgsrcdir}" \
> +		--list-file /dev/stdout "$1" \
> +		| sort -zu | tr '\0' '\n'
>   }
>   
>   strip_file() {
> @@ -58,9 +73,9 @@ strip_file() {
>   		# copy source files to debug directory
>   		local file dest t
>   		while IFS= read -r t; do
> -			file=${t/${dbgsrcdir}/"$srcdir"}
> -			dest="${dbgsrc/"$dbgsrcdir"/}$t"
> -			if ! [[ -f $dest ]]; then
> +			file="${srcdir}/${t}"
> +			dest="${dbgsrc}/${t}"
> +			if [[ -f "$file" ]] && ! [[ -f $dest ]]; then
>   				mkdir -p "${dest%/*}"
>   				cp -- "$file" "$dest"
>   			fi

Patch

diff --git a/scripts/libmakepkg/executable/debugedit.sh.in b/scripts/libmakepkg/executable/debugedit.sh.in
new file mode 100644
index 00000000..ec0ab814
--- /dev/null
+++ b/scripts/libmakepkg/executable/debugedit.sh.in
@@ -0,0 +1,38 @@ 
+#!/usr/bin/bash
+#
+#   debugedit.sh - Confirm presence of debugedit binary
+#
+#   Copyright (c) 2011-2022 Pacman Development Team <pacman-dev@lists.archlinux.org>
+#
+#   This program is free software; you can redistribute it and/or modify
+#   it under the terms of the GNU General Public License as published by
+#   the Free Software Foundation; either version 2 of the License, or
+#   (at your option) any later version.
+#
+#   This program is distributed in the hope that it will be useful,
+#   but WITHOUT ANY WARRANTY; without even the implied warranty of
+#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#   GNU General Public License for more details.
+#
+#   You should have received a copy of the GNU General Public License
+#   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+[[ -n "$LIBMAKEPKG_EXECUTABLE_DEBUGEDIT_SH" ]] && return
+LIBMAKEPKG_EXECUTABLE_DEBUGEDIT_SH=1
+
+LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
+
+source "$LIBRARY/util/message.sh"
+source "$LIBRARY/util/option.sh"
+
+executable_functions+=('executable_debugedit')
+
+executable_debugedit() {
+	if check_option "debug" "y"; then
+		if ! type -p debugedit >/dev/null; then
+			error "$(gettext "Cannot find the %s binary required for source files in debug packages.")" "debugedit"
+			return 1
+		fi
+	fi
+}
diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in
index 92a6fb15..84732c6f 100644
--- a/scripts/libmakepkg/tidy/strip.sh.in
+++ b/scripts/libmakepkg/tidy/strip.sh.in
@@ -36,8 +36,23 @@  build_id() {
 }
 
 source_files() {
-	LANG=C readelf "$1" --debug-dump 2>/dev/null | \
-		awk '/DW_AT_name +:/{name=$NF}/DW_AT_comp_dir +:/{{if (name == "<artificial>") next}{if (name !~ /^[<\/]/) {printf "%s/", $NF}}{print name}}'
+	# debugedit rewrites the binary. In most cases this is handled by the gcc
+	# prefix-map switches which rewrites $srcdir to $dbgsrcdir. debugedit will
+	# not do anything in those cases. However for binaries that does not have a
+	# prefix-map debugedit is going to modify the binary correcting the paths.
+	#
+	# From the mailing list discussion:
+	# * Without -b/-d: it just lists all source files.
+	# * With -b, it prints all source files rooted in the specified directory, but strips the
+	#   prefix from the output.
+	# * With both -b and -d, it replaces any base-dir prefixes with dest-dir
+	#   (modifying the binary), then prints all paths rooted in the dest-dir
+	#   (again, stripping the dest-dir prefix in the output).
+	LANG=C debugedit --no-recompute-build-id \
+		--base-dir "${srcdir}" \
+		--dest-dir "${dbgsrcdir}" \
+		--list-file /dev/stdout "$1" \
+		| sort -zu | tr '\0' '\n'
 }
 
 strip_file() {
@@ -58,9 +73,9 @@  strip_file() {
 		# copy source files to debug directory
 		local file dest t
 		while IFS= read -r t; do
-			file=${t/${dbgsrcdir}/"$srcdir"}
-			dest="${dbgsrc/"$dbgsrcdir"/}$t"
-			if ! [[ -f $dest ]]; then
+			file="${srcdir}/${t}"
+			dest="${dbgsrc}/${t}"
+			if [[ -f "$file" ]] && ! [[ -f $dest ]]; then
 				mkdir -p "${dest%/*}"
 				cp -- "$file" "$dest"
 			fi