From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnout Vandecappelle Date: Wed, 25 Jun 2014 22:31:20 +0200 Subject: [Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in In-Reply-To: <1403702174-23850-3-git-send-email-pascal.huerst@gmail.com> References: <1403702174-23850-1-git-send-email-pascal.huerst@gmail.com> <1403702174-23850-3-git-send-email-pascal.huerst@gmail.com> Message-ID: <53AB3198.9010908@mind.be> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net On 25/06/14 15:16, Pascal Huerst wrote: > Signed-off-by: Pascal Huerst > --- > Config.in | 14 ++++++++++++++ > Makefile | 5 +++++ > package/google-breakpad/Config.in | 5 ++++- > package/google-breakpad/gen-syms.sh | 25 +++++++++++++++++++++++++ > 4 files changed, 48 insertions(+), 1 deletion(-) > create mode 100755 package/google-breakpad/gen-syms.sh > > diff --git a/Config.in b/Config.in > index bf1ca86..4f2bd32 100644 > --- a/Config.in > +++ b/Config.in > @@ -480,6 +480,20 @@ config BR2_OPTIMIZE_S > > endchoice > > +config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES I don't really like the name of this option. What do you think of BR2_GOOGLE_BREAKPAD_GENSYMS_PATTERNS ? > + string "executables and libraries to be used by google-breakpad" > + depends on BR2_PACKAGE_GOOGLE_BREAKPAD Hm, this is counter-intuitive, though honestly I don't know how to improve it. The user would first have to go to the target packages and select google breakpad, and then return to the Build options menu at the top to set the patterns. Actually, this symbol could be defined inside the google-breakpad/Config.in, no? Though you probably got a comment before that that is not the right place :-) > + default "" > + help > + You may specify a space-separated list of binaries and libraries > + with full paths relative to $(TARGET_DIR) of which debug symbols Again trailing whitespace and line-wrapping. I think this text is slightly better: You may specify a space-separated list of glob patterns of binaries and libraries of which debug symbols will be dumped for further use with google breakpad. Th patterns are evaluated relative to $(TARGET_DIR). > + will be dumped for further use with google breakpad. > + > + A directory structure that can be used by minidump-stackwalk will > + be created at: > + > + $(STAGING_DIR)/usr/share/google-breakpad-symbols > + > config BR2_ENABLE_SSP > bool "build code with Stack Smashing Protection" > depends on BR2_TOOLCHAIN_HAS_SSP > diff --git a/Makefile b/Makefile > index 4fe370a..300e86e 100644 > --- a/Makefile > +++ b/Makefile > @@ -553,6 +553,11 @@ endif > ifeq ($(BR2_PACKAGE_PYTHON_PYC_ONLY)$(BR2_PACKAGE_PYTHON3_PYC_ONLY),y) > find $(TARGET_DIR)/usr/lib/ -name '*.py' -print0 | xargs -0 rm -f > endif > +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) > + $(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \ > + $(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES)) To be more compatible with Fabio's series that reworks this part of the infrastructure, it's better to define this as a new variable which is called here. That will make it easier to resolve the conflict between these two series. Fabio's series will also make it possible to define this completely inside the google-breakpad.mk. If the config variable contains a glob pattern, then this will be evaluated here (relative to the buildroot directory). Usually that will fail so it will be passed unexpanded, but perhaps it's safer to explicitly turn off globbing. Together with my other suggestions, this would become: ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) GOOGLE_BREAKPAD_INCLUDE_FILES = $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES)) ifneq ($(GOOGLE_BREAKPAD_INCLUDE_FILES),) # BR2_GOOGLE_BREAKPAD_INCLUDE_FILES may include glob files, so turn off globbing define GOOGLE_BREAKPAD_GEN_SYMS $(Q)set -o noglob; \ $(EXTRA_ENV) package/google-breakpad/gen-syms.sh \ $(STAGING_DIR) $(TARGET_DIR) $(GOOGLE_BREAKPAD_INCLUDE_FILES) endef endif endif > +endif > + > rm -rf $(TARGET_DIR)/usr/lib/luarocks > rm -rf $(TARGET_DIR)/usr/lib/perl5/$(PERL_VERSION)/$(PERL_ARCHNAME)/CORE > find $(TARGET_DIR)/usr/lib/perl5/ -name '*.bs' -print0 | xargs -0 rm -f > diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in > index fa1e8d9..99dc7a1 100644 > --- a/package/google-breakpad/Config.in > +++ b/package/google-breakpad/Config.in > @@ -11,7 +11,10 @@ config BR2_PACKAGE_GOOGLE_BREAKPAD > Breakpad can also write minidumps on request for programs that have not > crashed. > > - You may want to set BR2_ENABLE_DEBUG, in order to get useful results. > + Add all binaries and libraries you want to debug by google-breakpad to Trailing whitespace again. Also, I'd write Add all binaries and libraries that link with the google-breakpad client library and that you want to debug with minidump-stackwalk to BR2_GOOGLE_BREAKPAD_INCLUDE_FILES. > + BR2_GOOGLE_BREAKPAD_INCLUDE_FILES. > + > + You may also want to set BR2_ENABLE_DEBUG, in order to get useful results. > > http://code.google.com/p/google-breakpad/ > > diff --git a/package/google-breakpad/gen-syms.sh b/package/google-breakpad/gen-syms.sh > new file mode 100755 > index 0000000..d890752 > --- /dev/null > +++ b/package/google-breakpad/gen-syms.sh > @@ -0,0 +1,25 @@ > +#!/bin/sh > +STAGING_DIR="${1}" > +TARGET_DIR="${2}" > +shift 2 > + > +SYMBOLS_DIR="${STAGING_DIR}/usr/share/google-breakpad-symbols" > +rm -rf "${SYMBOLS_DIR}" > +mkdir -p "${SYMBOLS_DIR}/tmp" > + > +for FILE in $(eval ls "${TARGET_DIR}/${@}"); do This does not really work: if one of the patterns matches a directory, the directory will still be listed. So you have to use ls -d. However, I think the following is easier to understand, even if it's more wordy: for PATTERN in "$@"; do for FILE in ${TARGET_DIR}/${PATTERN}; do ... done done > + if [ -d "${FILE}" ]; then > + printf "Error: '%s' is a directory\n" "${FILE}" >&2 > + exit 1 > + fi > + if dump_syms "${FILE}" > "${SYMBOLS_DIR}/tmp/tmp.sym" 2>/dev/null; then Do we really want to redirect stderr here? Why do you put tmp.sym in a subdirectory, and not straignt into ${SYMBOLS_DIR}? > + HASH=$(head -n1 "${SYMBOLS_DIR}/tmp/tmp.sym" | cut -d ' ' -f 4); > + FILENAME=$(basename "$FILE"); I don't know if it is the official buildroot policy, but recently I've seen most scripts use "${FILE##*/}" instead of basename. Regards, Arnout > + mkdir -p "${SYMBOLS_DIR}/${FILENAME}/${HASH}" > + mv "${SYMBOLS_DIR}/tmp/tmp.sym" "${SYMBOLS_DIR}/${FILENAME}/${HASH}/${FILENAME}.sym"; > + else > + printf "Error dumping symbols for: '%s'\n" "${FILE}" >&2 > + exit 1 > + fi > +done > +rm -rf "${SYMBOLS_DIR}/tmp" > -- Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F