Patchwork [v4] Add helper script for glibc debugging

login
register
mail settings
Submitter Arjun Shankar
Date Sept. 30, 2019, 11:53 a.m.
Message ID <bf3b2e3a83b88f1ed55959468f27f6222487f218.camel@redhat.com>
Download mbox | patch
Permalink /patch/34725/
State New
Headers show

Comments

Arjun Shankar - Sept. 30, 2019, 11:53 a.m.
Hi Gabriel,

Thanks for writing this. The patch looks good to me.

I have one small suggestion. What do you think about this?:


The idea being that this will allow invoking (non-testcase) programs with
arguments, like:

./debugglibc.sh -i iconv/iconv_prog -f ASCII -t UTF-8 ascii.txt

I don't necessarily want this patch to go through more versions. It already
adds value and I think it's ready to go in today. Just wanted to enable
another usecase. Perhaps we can discuss it once the current version is in.

Cheers,
Arjun
Gabriel F. T. Gomes - Sept. 30, 2019, 1:30 p.m.
Hi, Arjun,

On Mon, 30 Sep 2019, Arjun Shankar wrote:

>The idea being that this will allow invoking (non-testcase) programs with
>arguments, like:

I haven't thought about this use case before, but it sounds good to me. :)

>./debugglibc.sh -i iconv/iconv_prog -f ASCII -t UTF-8 ascii.txt

I think we might need some sort of coordination between the optional
arguments to debugglibc.sh and these arguments to the program being tested.
Otherwise, the position of arguments such as "-e ENVVAR=VALUE" might break
this new use case.

For instance, currently, it doesn't matter if the optional arguments are
passed before or after the path to the test case, so, both:

./debugglibc.sh -b pthread_join nptl/tst-exec1
and
./debugglibc.sh nptl/tst-exec1 -b pthread_join

work.

>       TESTCASE=$$1
>+      COMMANDLINE="$$@"
>+      break
>       ;;

With this suggested change, that would no longer work, which is not
necessarily a problem, but caught my attention.

>+    --)
>+      shift
>+      TESTCASE=$$1
>+      COMMANDLINE="$$@"
>+      break
>+      ;;
>     *)

This option, on the other hand, has no conflicts with the current
behavior, but it would need some documentation in the --help message.

>I don't necessarily want this patch to go through more versions. It
>already adds value and I think it's ready to go in today. Just wanted to
>enable another usecase. Perhaps we can discuss it once the current
>version is in.

Since you already wrote the diff, would you also like to write the commit
message and send it as a patch?

Cheers,
Gabriel

Patch

diff --git a/Makefile b/Makefile
index a50fb64dc2..e832034ceb 100644
--- a/Makefile
+++ b/Makefile
@@ -255,8 +255,16 @@  do
     -s|--no-symbols-file)
       SYMBOLSFILE=false
       ;;
+    --)
+      shift
+      TESTCASE=$$1
+      COMMANDLINE="$$@"
+      break
+      ;;
     *)
       TESTCASE=$$1
+      COMMANDLINE="$$@"
+      break
       ;;
   esac
   shift
@@ -302,7 +310,7 @@  __ENVVARS__
 __SYMBOLSFILE__
 break _dl_start_user
 run --library-path $(rpath-link):$${BUILD_DIR}/nptl_db \
-__TESTCASE__ __DIRECT__
+__COMMANDLINE__ __DIRECT__
 __BREAKPOINTS__
 EOF
 }
@@ -311,7 +319,7 @@  EOF
 template | sed \
   -e "s|__ENVVARS__|$$ENVVARSCMD|" \
   -e "s|__SYMBOLSFILE__|$$SYMBOLSFILE|" \
-  -e "s|__TESTCASE__|$$TESTCASE|" \
+  -e "s|__COMMANDLINE__|$$COMMANDLINE|" \
   -e "s|__DIRECT__|$$DIRECT|" \
   -e "s|__BREAKPOINTS__|$$BREAKPOINTS|" \
   > $$CMD_FILE