[PATCH 1/2] unwind-libdw: add -kk/--stack-traces=+source option

[Dmitry V. Levin]

On Wed, Nov 08, 2023 at 09:25:58AM +0900, Masatake YAMATO wrote:
> This change adds source code information to stack traces printed with
> -k option if strace links to libdw.

Thanks, looks good, see my comments below.

[...]
> --- a/doc/strace.1.in
> +++ b/doc/strace.1.in
> @@ -38,7 +38,8 @@
>  strace \- trace system calls and signals
>  .SH SYNOPSIS
>  .SY strace
> -.if '@ENABLE_STACKTRACE_FALSE@'#' .OP \-ACdffhikqqrtttTvVwxxyyYzZ
> +.if '@ENABLE_STACKTRACE_FALSE@'#' .if '@USE_LIBDW_FALSE@'#' .OP \-ACdffhikKqqrtttTvVwxxyyYzZ
> +.if '@ENABLE_STACKTRACE_FALSE@'#' .if '@USE_LIBUNWIND_FALSE@'#' .OP \-ACdffhikqqrtttTvVwxxyyYzZ

I don't see why this change is needed.

>  .if '@ENABLE_STACKTRACE_TRUE@'#' .OP \-ACdffhiqqrtttTvVwxxyyYzZ
>  .OP \-a column
>  .OP \-b execve
> @@ -996,9 +997,16 @@ Print the syscall number.
>  .if '@ENABLE_STACKTRACE_FALSE@'#' .TP
>  .if '@ENABLE_STACKTRACE_FALSE@'#' .B \-k
>  .if '@ENABLE_STACKTRACE_FALSE@'#' .TQ
> -.if '@ENABLE_STACKTRACE_FALSE@'#' .B \-\-stack\-traces
> +.if '@ENABLE_STACKTRACE_FALSE@'#' .B \-\-stack\-traces [= symbol ]
>  .if '@ENABLE_STACKTRACE_FALSE@'#' Print the execution stack trace of the traced
>  .if '@ENABLE_STACKTRACE_FALSE@'#' processes after each system call.
> +.if '@USE_LIBDW_FALSE@'#' .TP
> +.if '@USE_LIBDW_FALSE@'#' .B \-kk
> +.if '@USE_LIBDW_FALSE@'#' .TQ
> +.if '@USE_LIBDW_FALSE@'#' .B \-\-stack\-traces [= +source ]

I'm not sure that "+" in "--stack-traces=+source" is a really good idea.
Wouldn't "--stack-traces=source" look easier to use?

> @@ -2453,12 +2464,47 @@ init(int argc, char *argv[])
>  			break;
>  		case 'k':
>  #ifdef ENABLE_STACKTRACE
> -			stack_trace_enabled = true;
> +			switch (stack_trace_mode) {
> +			case STACK_TRACE_OFF:
> +				stack_trace_mode = STACK_TRACE_ON;
> +				break;
> +			case STACK_TRACE_ON:
> +# ifdef USE_LIBDW
> +				stack_trace_mode = STACK_TRACE_WITH_SRCINFO;
> +# else
> +				error_msg_and_die("Stack traces (-kk "

I'd rather say "Stack traces with source line information" here.

> +						  "option) are not supported by this "
> +						  "build of strace");
> +# endif	/* USE_LIBDW */
> +				break;
> +			default:
> +				error_msg_and_die("Too many -k options");

Note that this line is not covered by tests.

> +			}
> +#else
> +			error_msg_and_die("Stack traces (-k "
> +					  "option) are not supported by this "
> +					  "build of strace");
> +#endif /* ENABLE_STACKTRACE */
> +			break;
> +		case GETOPT_STACK:
> +#ifdef ENABLE_STACKTRACE
> +			if (optarg == NULL || strcmp(optarg, "symbol") == 0)
> +				stack_trace_mode = STACK_TRACE_ON;
> +			else if (strcmp(optarg, "+source") == 0) {

Likewise, note that this line is not covered by tests.

> +# ifdef USE_LIBDW
> +				stack_trace_mode = STACK_TRACE_WITH_SRCINFO;
> +# else
> +				error_msg_and_die("Stack traces (--stack-traces=+source "
> +						  "option) are not supported by this "
> +						  "build of strace");
> +# endif /* USE_LIBDW */
> +			} else
> +				error_opt_arg(c, lopt, optarg);

Likewise, note that this line is not covered by tests.

>  #else
> -			error_msg_and_die("Stack traces (-k/--stack-traces "
> +			error_msg_and_die("Stack traces (--stack-traces[=symbol] "
>  					  "option) are not supported by this "
>  					  "build of strace");
> -#endif
> +#endif /* ENABLE_STACKTRACE */
>  			break;
>  		case GETOPT_KILL_ON_EXIT:
>  			opt_kill_on_exit = true;
> @@ -2791,8 +2837,11 @@ init(int argc, char *argv[])
>  		if (iflag)
>  			error_msg("-i/--instruction-pointer has no effect "
>  				  "with -c/--summary-only");
> -		if (stack_trace_enabled)
> -			error_msg("-k/--stack-traces has no effect "
> +		if (stack_trace_mode == STACK_TRACE_ON)
> +			error_msg("-k/--stack-traces[=symbol] has no effect "
> +				  "with -c/--summary-only");
> +		if (stack_trace_mode == STACK_TRACE_WITH_SRCINFO)
> +			error_msg("-kk/--stack-traces=+source has no effect "
>  				  "with -c/--summary-only");
>  		if (nflag)
>  			error_msg("-n/--syscall-number has no effect "

I'm not sure this change of diagnostics is really needed.

[...]
> @@ -195,14 +201,24 @@ frame_callback(Dwfl_Frame *state, void *arg)
>  			const char *symname = NULL;
>  			GElf_Sym sym;
>  			Dwarf_Addr true_offset = pc;
> +			const char *source_filename = NULL;
> +			int source_line = 0;
>  
>  			modname = dwfl_module_info(mod, NULL, NULL, NULL, NULL,
>  						   NULL, NULL, NULL);
>  			symname = dwfl_module_addrinfo(mod, pc, &off, &sym,
>  						       NULL, NULL, NULL);
>  			dwfl_module_relocate_address(mod, &true_offset);
> +			if (with_srcinfo) {
> +				Dwfl_Line *dwfl_line;
> +
> +				dwfl_line = dwfl_module_getsrc(mod, pc);
> +				if (dwfl_line)
> +					source_filename = dwfl_lineinfo(dwfl_line, NULL,
> +									&source_line, NULL, NULL, NULL);

This line is too long, please fold it.

> +			}
>  			user_data->call_action(user_data->data, modname, symname,
> -					       off, true_offset);
> +					       off, true_offset, source_filename, source_line);

Likewise.


-- 
ldv