[llvm-symbolizer]Fix printing of malformed address values not passed via stdin

[TH3CHARLie]

Summary

relates https://bugs.llvm.org/show_bug.cgi?id=44443

Adding missing newline when printing bad input values.

Fix testcase

[jhenderson]

Thanks for the patch! Could you add an additional test case for passing the values via a response file, rather than directly on the command-line, i.e. llvm-symbolizer <some args> @response.txt where response.txt contains "some text", inctwo and "some text2" etc.

Also, I think it would be good to expand invalid-input-address.test to cover this. In fact, you might want to put both your new case and something equivalent to the existing one in sym.test in the same place. That way, we're not lumping multiple different kinds of checks into the same test file.

llvm/test/tools/llvm-symbolizer/sym.test

34–36

You should probably add the same test case as you have above for llvm-addr2line (the GNU-compatible version of llvm-symbolizer).

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

332–338

LLVM style for comments is a leading capital letter and trailing full stop:
// Strip newline characters.

334

Don't forget that on some systems (i.e. Windows), new lines end in '\r\n'. You need to make sure both are stripped.

[TH3CHARLie]

In D72313#1807518, @jhenderson wrote:

Also, I think it would be good to expand invalid-input-address.test to cover this. In fact, you might want to put both your new case and something equivalent to the existing one in sym.test in the same place. That way, we're not lumping multiple different kinds of checks into the same test file.

I'm not quite sure about this so I'd like to confirm my understanding again before updating. Feel free to correct me if I misunderstand this: adding new tests(instead of moving existing ones in sym.test) to invalid-input-address.test that checks for invalid value like the sometext used in sym.test. And the testcases cover command line, ordinary textual file and response file.

[jhenderson]

In D72313#1807539, @TH3CHARLie wrote:

In D72313#1807518, @jhenderson wrote:

Also, I think it would be good to expand invalid-input-address.test to cover this. In fact, you might want to put both your new case and something equivalent to the existing one in sym.test in the same place. That way, we're not lumping multiple different kinds of checks into the same test file.

I'm not quite sure about this so I'd like to confirm my understanding again before updating. Feel free to correct me if I misunderstand this: adding new tests(instead of moving existing ones in sym.test) to invalid-input-address.test that checks for invalid value like the sometext used in sym.test. And the testcases cover command line, ordinary textual file and response file.

That's correct. It tests the three different ways you can get inputs into llvm-symbolizer (command-line, response file, stdin). You may want to include llvm-addr2line and llvm-symbolizer versions too.

[TH3CHARLie]

Update according to comments

[TH3CHARLie]

In D72313#1807552, @jhenderson wrote:

In D72313#1807539, @TH3CHARLie wrote:

In D72313#1807518, @jhenderson wrote:

Also, I think it would be good to expand invalid-input-address.test to cover this. In fact, you might want to put both your new case and something equivalent to the existing one in sym.test in the same place. That way, we're not lumping multiple different kinds of checks into the same test file.

I'm not quite sure about this so I'd like to confirm my understanding again before updating. Feel free to correct me if I misunderstand this: adding new tests(instead of moving existing ones in sym.test) to invalid-input-address.test that checks for invalid value like the sometext used in sym.test. And the testcases cover command line, ordinary textual file and response file.

That's correct. It tests the three different ways you can get inputs into llvm-symbolizer (command-line, response file, stdin). You may want to include llvm-addr2line and llvm-symbolizer versions too.

Well, if so, doesn't it duplicates what has been tested in sym.test?

[TH3CHARLie]

llvm/test/tools/llvm-symbolizer/Inputs/response_addr.txt

2

I'll update a newline here

[jhenderson]

In D72313#1807560, @TH3CHARLie wrote:

Well, if so, doesn't it duplicates what has been tested in sym.test?

Yes, but sym.test is not a very good test as it tries to test too many different things at once, which means it's hard to actually tell what it's testing or to maintain the test. I'd like to split it up/rewrite it/delete it etc at some point, although it's not high on my priority list. If we keep adding things to that test, it will actually make things worse. By putting this testing in the other test case, we have all our testing for this piece of functionality (i.e. what to do about unknown addresses) in one place, making it easy to find etc.

Does that make sense?

[TH3CHARLie]

In D72313#1807564, @jhenderson wrote:

In D72313#1807560, @TH3CHARLie wrote:

Well, if so, doesn't it duplicates what has been tested in sym.test?

Yes, but sym.test is not a very good test as it tries to test too many different things at once, which means it's hard to actually tell what it's testing or to maintain the test. I'd like to split it up/rewrite it/delete it etc at some point, although it's not high on my priority list. If we keep adding things to that test, it will actually make things worse. By putting this testing in the other test case, we have all our testing for this piece of functionality (i.e. what to do about unknown addresses) in one place, making it easy to find etc.

Does that make sense?

Thanks for the explanation! Now it's much more clear to me

[jhenderson]

llvm/test/tools/llvm-symbolizer/Inputs/response_addr.txt

2

A more appropriate thing to do is have these tests generate their input files on the fly with something like the following:

# RUN: echo "some text" 0x40054d "some text2" > %t.rsp
# RUN: llvm-symbolizer @%t.rsp ...

A similar pattern can be used for the stdin reading.

(note: I'm not sure if those quotes will appear in the output or not - you'd best double-check that).

llvm/test/tools/llvm-symbolizer/sym.test

21–22

If you're adding the test cases in a new test, you don't need these cases in this test too.

[TH3CHARLie]

update invalid-input-address.test

[jhenderson]

llvm/test/tools/llvm-symbolizer/invalid-input-address.test

3–4 ↗

(On Diff #236565)

I don't think the "good" address is important here, so you can remove it. Also, the comment is referring to the original RUN line that was there before, so move the echo runs to nearer where they're used.

6 ↗

(On Diff #236565)

I think LARGE-ADDR would be preferable here.

8 ↗

(On Diff #236565)

You should add a comment before this line showing what these are intended to test.

8–10 ↗

(On Diff #236565)

print-address isn't important for these checks (no address is applicable for bad inputs).

Also, prefer '--' for options isntead of '-'.

Also, I'd recommend naming the CHECK prefixes here something like "LLVM" and llvm-addr2line's to "GNU".

16–18 ↗

(On Diff #236565)

Let's put the checks nearer their corresponding RUN lines, like this:

RUN: llvm-symbolizer ... | FileCheck --check-prefix=LARGE-ADDR %s

LARGE-ADDR: ...

RUN: llvm-symbolizer ... | FileCheck --check-prefix=BAD-INPUT %s

BAD-INPUT: ...

[TH3CHARLie]

Update invalid-input-address.test according to comments

[jhenderson]

llvm/test/tools/llvm-symbolizer/invalid-input-address.test

3 ↗

(On Diff #236733)

Could you change this to "--check-prefix" instead of "-check-prefix" whilst you're here, please? Same comment applies throughout the test.

17 ↗

(On Diff #236733)

Nit: add some spaces here, so that it lines up with the check below, like the LARGE-ADDR case:

LLVM-BAD-INPUT:      some text
LLVM-BAD-INPUT-NEXT: some text2

25–26 ↗

(On Diff #236733)

This was silly of me, sorry, I should have noticed earlier - you don't need separate CHECK sets for llvm-symbolizer and llvm-addr2line, because they test the same things:

Here's what I'd recommend:

RUN: llvm-symbolizer ... | FileCheck %s --check-prefix=BAD-INPUT
# <other llvm-symbolizer cases>

RUN: llvm-addr2line ... | FileCheck %s --check-prefix=BAD-INPUT
# <other llvm-addr2line cases>

BAD-INPUT: ...

[TH3CHARLie]

response to reviews

[jhenderson]

Assuming the tests all pass, LGTM, thanks! Do you have commit access yet?

[TH3CHARLie]

In D72313#1809694, @jhenderson wrote:

Assuming the tests all pass, LGTM, thanks! Do you have commit access yet?

Thanks for the review! All tests passed and I've recently gained commit access and want to try to commit this. If there's nothing else, I will commit this

[jhenderson]

In D72313#1809709, @TH3CHARLie wrote:

In D72313#1809694, @jhenderson wrote:

Assuming the tests all pass, LGTM, thanks! Do you have commit access yet?

Thanks for the review! All tests passed and I've recently gained commit access and want to try to commit this. If there's nothing else, I will commit this

Great, good luck! Only thing I would say is to make sure to not forget to include "Differential Revision: https://reviews.llvm.org/D72313" as a line in your commit message so that this review gets auto-closed.

llvm/test/tools/llvm-symbolizer/Inputs/response_addr.txt

+"some text" 0x40054d "some text2"
 No newline at end of file

llvm/test/tools/llvm-symbolizer/sym.test

 #int main () {
 #  int x = 1;
 #  return inc(&x);
 #}
 #
 #Build as : clang -g -O2 addr.c
 
 RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe "some text" 0x40054d "some text2" | FileCheck %s
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe @%p/Inputs/response_addr.txt | FileCheck %s
 RUN: llvm-symbolizer -addresses -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
 RUN: llvm-symbolizer -a -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
 RUN: llvm-symbolizer -inlining -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s 
 RUN: llvm-symbolizer -inlining -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: llvm-symbolizer -inlines -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: llvm-symbolizer -inlines -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: llvm-symbolizer -i -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: llvm-symbolizer -i -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: echo "0x1" > %t.input
 RUN: llvm-symbolizer -obj=%p/Inputs/zero < %t.input | FileCheck -check-prefix="ZERO" %s
 
 RUN: llvm-addr2line -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix=A2L %s
+RUN: llvm-addr2line -obj=%p/Inputs/addr.exe "some text" 0x40054d "some text2" | FileCheck -check-prefix=A2L %s
+RUN: llvm-addr2line -obj=%p/Inputs/addr.exe @%p/Inputs/response_addr.txt | FileCheck -check-prefix=A2L %s
 RUN: llvm-addr2line -a -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2L,A2L_A %s
 RUN: llvm-addr2line -f -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2L,A2L_F %s
 RUN: llvm-addr2line -i -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2L,A2L_I %s
 RUN: llvm-addr2line -fi -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2L,A2L_F,A2L_I,A2L_FI %s
 
 RUN: llvm-addr2line -pa -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_A %s
 RUN: llvm-addr2line -pf -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_F %s
 RUN: llvm-addr2line -paf -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_AF %s
 RUN: llvm-addr2line -pai -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_A,A2LP_I %s
 RUN: llvm-addr2line -pfi -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_F,A2LP_FI %s
 RUN: llvm-addr2line -pafi -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefixes=A2LP,A2LP_AF,A2LP_FI %s
 
-#CHECK: some text
-#CHECK: 0x40054d
-#CHECK: main
-#CHECK: {{[/\]+}}tmp{{[/\]+}}x.c:14:0
-#CHECK: some text2
+# CHECK: some text
+# CHECK-NEXT: 0x40054d
+# CHECK-NEXT: inctwo
+# CHECK-NEXT: {{[/\]+}}tmp{{[/\]+}}x.c:3:3
+# CHECK-NEXT: inc
+# CHECK-NEXT: {{[/\]+}}tmp{{[/\]+}}x.c:7:0
+# CHECK-NEXT: main
+# CHECK-NEXT: {{[/\]+}}tmp{{[/\]+}}x.c:14:0
+# CHECK-EMPTY:
+# CHECK-NEXT: some text2
 #
 #PRETTY: some text
 #PRETTY: {{[0x]+}}40054d: inctwo at {{[/\]+}}tmp{{[/\]+}}x.c:3:3
 #PRETTY:  (inlined by) inc at {{[/\]+}}tmp{{[/\]+}}x.c:7:0
 #PRETTY   (inlined by) main at {{[/\]+}}tmp{{[/\]+}}x.c:14:0
 #PRETTY: some text2
 #
 #ZERO: ??

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

 #include "llvm/DebugInfo/Symbolize/Symbolize.h"
 #include "llvm/Support/COM.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include <algorithm>
 #include <cstdio>
 #include <cstring>
 #include <string>
 
 using namespace llvm;
 using namespace symbolize;
 
 static cl::opt<bool>

 }
 
 static void symbolizeInput(StringRef InputString, LLVMSymbolizer &Symbolizer,
                            DIPrinter &Printer) {
   Command Cmd;
   std::string ModuleName;
   uint64_t Offset = 0;
   if (!parseCommand(StringRef(InputString), Cmd, ModuleName, Offset)) {
-    outs() << InputString;
+    outs() << InputString << "\n";
     return;
   }
 
   if (ClPrintAddress) {
     outs() << "0x";
     outs().write_hex(Offset);
     StringRef Delimiter = ClPrettyPrint ? ": " : "\n";
     outs() << Delimiter;
DIPrinter Printer(outs(), ClPrintFunctions != FunctionNameKind::None,
                     ClPrettyPrint, ClPrintSourceContextLines, ClVerbose,
                     ClBasenames, ClOutputStyle);
 
   if (ClInputAddresses.empty()) {
     const int kMaxInputStringLength = 1024;
     char InputString[kMaxInputStringLength];
 
     while (fgets(InputString, sizeof(InputString), stdin)) {
-      symbolizeInput(InputString, Symbolizer, Printer);
+      // Strip newline characters.
+      std::string StrippedInputString(InputString);
+      StrippedInputString.erase(
+          std::remove_if(StrippedInputString.begin(), StrippedInputString.end(),
+                         [](char c) { return c == '\r' || c == '\n'; }),
+          StrippedInputString.end());
+      symbolizeInput(StrippedInputString, Symbolizer, Printer);
       outs().flush();
     }
   } else {
     for (StringRef Address : ClInputAddresses)
       symbolizeInput(Address, Symbolizer, Printer);
   }
 
   return 0;
 }