[PATCH 0/2] addr2line: new option -n to add a newline at the end

[PATCH 0/2] addr2line: new option -n to add a newline at the end

[Maurizio Papini]

This series adds a new option to addr2line (-n) to append a newline
after the last informative one.

This new option is helpful for using a running addr2line process and
performing queries, in particular when the option -i is requested: the
additional empty line can be used to mark the end of the inlined functions
lists so that an application can get the output without defining a timeout.

The first patch adds the new option while the second one adds the relative
test.

Let me know what you think.

Maurizio


Maurizio Papini (2):
  addr2line: new option -n to add a newline at the end
  addr2line: test to check -n option

 binutils/addr2line.c                          | 11 +++++++++--
 binutils/doc/binutils.texi                    |  5 +++++
 binutils/testsuite/binutils-all/addr2line.exp | 10 ++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.38.1

[PATCH 1/2] addr2line: new option -n to add a newline at the end

[Maurizio Papini]

---
 binutils/addr2line.c       | 11 +++++++++--
 binutils/doc/binutils.texi |  5 +++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/binutils/addr2line.c b/binutils/addr2line.c
index baf16716182..d9338e9f1ea 100644
--- a/binutils/addr2line.c
+++ b/binutils/addr2line.c
@@ -45,7 +45,7 @@ static bool with_functions;	/* -f, show function names.  */
 static bool do_demangle;	/* -C, demangle names.  */
 static bool pretty_print;	/* -p, print on one line.  */
 static bool base_names;		/* -s, strip directory names.  */
-
+static bool add_newline;        /* -n, add a newline at the end. */
 /* Flags passed to the name demangler.  */
 static int demangle_flags = DMGL_PARAMS | DMGL_ANSI;
 
@@ -70,6 +70,7 @@ static struct option long_options[] =
   {"no-recursion-limit", no_argument, NULL, 'r'},  
   {"section", required_argument, NULL, 'j'},
   {"target", required_argument, NULL, 'b'},
+  {"add-newline", no_argument, NULL, 'n'},
   {"help", no_argument, NULL, 'H'},
   {"version", no_argument, NULL, 'V'},
   {0, no_argument, 0, 0}
@@ -102,6 +103,7 @@ usage (FILE *stream, int status)
   -C --demangle[=style]  Demangle function names\n\
   -R --recurse-limit     Enable a limit on recursion whilst demangling.  [Default]\n\
   -r --no-recurse-limit  Disable a limit on recursion whilst demangling\n\
+  -n --add-newline       Add a newline at the end of the output\n\
   -h --help              Display this information\n\
   -v --version           Display the program's version\n\
 \n"));
@@ -417,6 +419,8 @@ translate_addresses (bfd *abfd, asection *section)
             }
 	}
 
+      if(add_newline)
+        printf("\n");
       /* fflush() is essential for using this command as a server
          child process that reads addresses from a pipe and responds
          with line number information, processing one address at a
@@ -505,7 +509,7 @@ main (int argc, char **argv)
   file_name = NULL;
   section_name = NULL;
   target = NULL;
-  while ((c = getopt_long (argc, argv, "ab:Ce:rRsfHhij:pVv", long_options, (int *) 0))
+  while ((c = getopt_long (argc, argv, "ab:Ce:rRsfnHhij:pVv", long_options, (int *) 0))
 	 != EOF)
     {
       switch (c)
@@ -564,6 +568,9 @@ main (int argc, char **argv)
 	case 'j':
 	  section_name = optarg;
 	  break;
+  case 'n':
+    add_newline = true;
+    break;
 	default:
 	  usage (stderr, 1);
 	  break;
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index c9d52d7c92f..0940708d77e 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -3976,6 +3976,7 @@ addr2line [@option{-a}|@option{--addresses}]
           [@option{-f}|@option{--functions}] [@option{-s}|@option{--basename}]
           [@option{-i}|@option{--inlines}]
           [@option{-p}|@option{--pretty-print}]
+          [@option{-n}|@option{--add-newline}]
           [@option{-j}|@option{--section=}@var{name}]
           [@option{-H}|@option{--help}] [@option{-V}|@option{--version}]
           [addr addr @dots{}]
@@ -4102,6 +4103,10 @@ Make the output more human friendly: each location are printed on one line.
 If option @option{-i} is specified, lines for all enclosing scopes are
 prefixed with @samp{(inlined by)}.
 
+@item -n
+@itemx --add-newline
+Add a newline at the end of the output.
+
 @item -r
 @itemx -R
 @itemx --recurse-limit
-- 
2.38.1

[PATCH 2/2] addr2line: test to check -n option

[Maurizio Papini]

---
 binutils/testsuite/binutils-all/addr2line.exp | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/binutils/testsuite/binutils-all/addr2line.exp b/binutils/testsuite/binutils-all/addr2line.exp
index 957ae55df33..59c0924412c 100644
--- a/binutils/testsuite/binutils-all/addr2line.exp
+++ b/binutils/testsuite/binutils-all/addr2line.exp
@@ -61,6 +61,16 @@ if ![regexp -line "^(\[0-9a-fA-F\]+)? +\[Tt\] ${dot}fn" $output contents] then {
 	pass "$testname -f option"
     }
 
+#testcase for -n option: check the extra newline
+#Using the same fn function address used in -f option.
+    set got [binutils_run $ADDR2LINE "-n -f -e tmpdir/testprog$exe [lindex $list 0]"]
+    set want "fn\n$srcdir/$subdir/testprog.c:\[0-9\]+\n\n"
+    if ![regexp $want $got] then {
+	fail "$testname -fn option $got\n"
+    } else {
+	pass "$testname -fn option"
+    }
+
 #testcase for -s option.
 #Using the same fn function address used in -f option.
     set got [binutils_run $ADDR2LINE "-s -e tmpdir/testprog$exe [lindex $list 0]"]
-- 
2.38.1

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Maurizio Papini]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hi All,

Do you think this should go through an RFC? Do you have any thoughts?

Thanks for your time.

BR,
Maurizio

On Mon, Dec 19, 2022 at 2:53 PM Maurizio Papini <mpapini@redhat.com> wrote:

> This series adds a new option to addr2line (-n) to append a newline
> after the last informative one.
>
> This new option is helpful for using a running addr2line process and
> performing queries, in particular when the option -i is requested: the
> additional empty line can be used to mark the end of the inlined functions
> lists so that an application can get the output without defining a timeout.
>
> The first patch adds the new option while the second one adds the relative
> test.
>
> Let me know what you think.
>
> Maurizio
>
>
> Maurizio Papini (2):
>   addr2line: new option -n to add a newline at the end
>   addr2line: test to check -n option
>
>  binutils/addr2line.c                          | 11 +++++++++--
>  binutils/doc/binutils.texi                    |  5 +++++
>  binutils/testsuite/binutils-all/addr2line.exp | 10 ++++++++++
>  3 files changed, 24 insertions(+), 2 deletions(-)
>
> --
> 2.38.1
>
>

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Jan Beulich]

On 09.01.2023 17:04, Maurizio Papini via Binutils wrote:
> Do you think this should go through an RFC? Do you have any thoughts?

Well, I'm of two minds here, which so far kept me from responding: On one
hand an option that's useful to someone is probably a good thing. Otoh an
option to control a single newline character seems a little too fine
grained to me. Plus I'm a little concerned of burning a short option for
this (niche?) issue. Since you say it's specifically an issue with -i,
would it be an option to add a long-only option providing the intended
variant of behavior, i.e. combining what would (aiui) be "-i -n" with the
current proposal?

Jan

> On Mon, Dec 19, 2022 at 2:53 PM Maurizio Papini <mpapini@redhat.com> wrote:
> 
>> This series adds a new option to addr2line (-n) to append a newline
>> after the last informative one.
>>
>> This new option is helpful for using a running addr2line process and
>> performing queries, in particular when the option -i is requested: the
>> additional empty line can be used to mark the end of the inlined functions
>> lists so that an application can get the output without defining a timeout.
>>
>> The first patch adds the new option while the second one adds the relative
>> test.
>>
>> Let me know what you think.
>>
>> Maurizio
>>
>>
>> Maurizio Papini (2):
>>   addr2line: new option -n to add a newline at the end
>>   addr2line: test to check -n option
>>
>>  binutils/addr2line.c                          | 11 +++++++++--
>>  binutils/doc/binutils.texi                    |  5 +++++
>>  binutils/testsuite/binutils-all/addr2line.exp | 10 ++++++++++
>>  3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> --
>> 2.38.1
>>
>>

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Maurizio Papini]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

Thanks for your feedback Jan.

Yes, I understand that one option for a single newline smells fulsome but
the
idea is to use it to mark the end of output when addr2line is used as a
"server"
process: right now I see only the "-i" use case, but there could be others
in the
future for new options.  Said that I would propose adding the "add-newline"
long
option only to trivially add a new line, that could be reused for other
output.
What do you think?

Maurizio

On Mon, Jan 9, 2023 at 5:17 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 09.01.2023 17:04, Maurizio Papini via Binutils wrote:
> > Do you think this should go through an RFC? Do you have any thoughts?
>
> Well, I'm of two minds here, which so far kept me from responding: On one
> hand an option that's useful to someone is probably a good thing. Otoh an
> option to control a single newline character seems a little too fine
> grained to me. Plus I'm a little concerned of burning a short option for
> this (niche?) issue. Since you say it's specifically an issue with -i,
> would it be an option to add a long-only option providing the intended
> variant of behavior, i.e. combining what would (aiui) be "-i -n" with the
> current proposal?
>
> Jan
>
> > On Mon, Dec 19, 2022 at 2:53 PM Maurizio Papini <mpapini@redhat.com>
> wrote:
> >
> >> This series adds a new option to addr2line (-n) to append a newline
> >> after the last informative one.
> >>
> >> This new option is helpful for using a running addr2line process and
> >> performing queries, in particular when the option -i is requested: the
> >> additional empty line can be used to mark the end of the inlined
> functions
> >> lists so that an application can get the output without defining a
> timeout.
> >>
> >> The first patch adds the new option while the second one adds the
> relative
> >> test.
> >>
> >> Let me know what you think.
> >>
> >> Maurizio
> >>
> >>
> >> Maurizio Papini (2):
> >>   addr2line: new option -n to add a newline at the end
> >>   addr2line: test to check -n option
> >>
> >>  binutils/addr2line.c                          | 11 +++++++++--
> >>  binutils/doc/binutils.texi                    |  5 +++++
> >>  binutils/testsuite/binutils-all/addr2line.exp | 10 ++++++++++
> >>  3 files changed, 24 insertions(+), 2 deletions(-)
> >>
> >> --
> >> 2.38.1
> >>
> >>
>
>

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Jan Beulich]

On 10.01.2023 15:17, Maurizio Papini wrote:
> Thanks for your feedback Jan.
> 
> Yes, I understand that one option for a single newline smells fulsome but
> the
> idea is to use it to mark the end of output when addr2line is used as a
> "server"
> process: right now I see only the "-i" use case, but there could be others
> in the
> future for new options.  Said that I would propose adding the "add-newline"
> long
> option only to trivially add a new line, that could be reused for other
> output.
> What do you think?

I don't really like the idea, but I can see this being a compromise to cover
your use case. Yet in your reply to Nick you indicate you might go the route
he proposed, which I understand would allow us to get away without any such
option. If, however, an option is still needed from your pov, then please
make its name more meaningful than just "add-newline" - that doesn't tell at
all what the real purpose is. Maybe "separate-<whatever>", with <whatever>
suitably replaced, and then possibly also (in the future, not necessarily
right now) allowing to specify a character to use as the separator in place
of the (default) newline.

Jan

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Nick Clifton]

Hi Maurizio,

> Do you think this should go through an RFC? Do you have any thoughts?

I am sorry - I totally missed this patch submission. :-(

>> This series adds a new option to addr2line (-n) to append a newline
>> after the last informative one.

I have to say that I am kind of with Jan on this one - it seems like
a lot of effort to fix a small problem.  Isn't there an easier way
that would not involve patching addr2line ?  For example if you use
the -a/--addresses option then each decoded address will be prefixed
with a line containing a simple hex number.  This would allow a consumer
of addr2line's output to detect the start of each decoded address.


> the
> additional empty line can be used to mark the end of the inlined functions
> lists so that an application can get the output without defining a timeout.

I am not sure what you are getting at here.  Are you saying that addr2line
can get stuck and so a timeout is needed in order to be to distinguish between
addr2line being slow and addr2line being stuck ?  If so then this sounds
like a more serious problem that needs to be addressed by something other
than just adding a blank line to the output.

Cheers
   Nick

Re: [PATCH 0/2] addr2line: new option -n to add a newline at the end

[Maurizio Papini]

Hi Nick and thanks for your time.

> I am sorry - I totally missed this patch submission. :-(

The patch is definitely not urgent, trivial stuff indeed, and my submission day
was so close to the end of 2022.

> I am not sure what you are getting at here.  Are you saying that addr2line
> can get stuck and so a timeout is needed in order to be to distinguish between
> addr2line being slow and addr2line being stuck ?  If so then this sounds
> like a more serious problem that needs to be addressed by something other
> than just adding a blank line to the output.

No, I didn't see this kind of problem. I'm talking about this use case:
addr2line as a process, with "-i" option, that is reading from stdin
working on a
big binary, say like the Linux kernel with debug info (~400M+), so that spawning
the process once is definitely better than doing it every time.

> I have to say that I am kind of with Jan on this one - it seems like
> a lot of effort to fix a small problem.  Isn't there an easier way
> that would not involve patching addr2line ?  For example if you use
> the -a/--addresses option then each decoded address will be prefixed
> with a line containing a simple hex number.  This would allow a consumer
> of addr2line's output to detect the start of each decoded address.

I'm thinking about using your suggestion (thanks): with -a the address is added
as a prefix as you said, while it would have been useful to have it at
the end :-),
but querying two times...

Maurizio

Maurizio


On Tue, Jan 10, 2023 at 1:19 PM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi Maurizio,
>
> > Do you think this should go through an RFC? Do you have any thoughts?
>
> I am sorry - I totally missed this patch submission. :-(
>
> >> This series adds a new option to addr2line (-n) to append a newline
> >> after the last informative one.
>
> I have to say that I am kind of with Jan on this one - it seems like
> a lot of effort to fix a small problem.  Isn't there an easier way
> that would not involve patching addr2line ?  For example if you use
> the -a/--addresses option then each decoded address will be prefixed
> with a line containing a simple hex number.  This would allow a consumer
> of addr2line's output to detect the start of each decoded address.
>
>
> > the
> > additional empty line can be used to mark the end of the inlined functions
> > lists so that an application can get the output without defining a timeout.
>
> I am not sure what you are getting at here.  Are you saying that addr2line
> can get stuck and so a timeout is needed in order to be to distinguish between
> addr2line being slow and addr2line being stuck ?  If so then this sounds
> like a more serious problem that needs to be addressed by something other
> than just adding a blank line to the output.
>
> Cheers
>    Nick
>
>