[v4] Add helper script for glibc debugging

Message ID bf3b2e3a83b88f1ed55959468f27f6222487f218.camel@redhat.com
State Superseded
Headers

Commit Message

Arjun Shankar Sept. 30, 2019, 11:53 a.m. UTC
  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
  

Comments

Gabriel F. T. Gomes Sept. 30, 2019, 1:30 p.m. UTC | #1
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