[Lldb-commits] [PATCH] D13058: LLDB-MI: Bug when evaluating strings containing characters from non-ascii range
Ilia K via lldb-commits
lldb-commits at lists.llvm.org
Wed Sep 23 00:48:32 PDT 2015
ki.stfu requested changes to this revision.
ki.stfu added a comment.
Please, next time make a patch with full context:
svn diff --diff-cmd diff -x "-U9999" > mypatch.txt
This will help us to reduce the review time.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:14
@@ +13,3 @@
+ # evaluates array when char-array-as-string is off
+ def eval_array(self, name, length, typ):
+ self.runCmd("-var-create - * " + name)
----------------
Probably it should be named like eval_and_check_array()
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:16-19
@@ +15,6 @@
+ self.runCmd("-var-create - * " + name)
+ self.expect("\^done,name=\"var\d+\",numchild=\"" +
+ str(length) +
+ "\",value=\"\[" + str(length) + "\]\",type=\"" +
+ typ + " \[" + str(length) + "\]\",thread-id=\"1\",has_more=\"0\"")
+
----------------
Please use '%' operator to format the string.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:21
@@ +20,3 @@
+
+ # evaluates array or pointer when char-array-as-string is on
+ def eval_ap(self, name, value, sum_part, typ):
----------------
This comment deceives us because you use eval_ap on the line #56 at the time when char-array-as-string is "off".
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:22
@@ +21,3 @@
+ # evaluates array or pointer when char-array-as-string is on
+ def eval_ap(self, name, value, sum_part, typ):
+ self.runCmd("-var-create - * " + name)
----------------
What is ap stands for?
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:26
@@ +25,3 @@
+ value +
+ "\\\\\\\"\\\\\\\\t\\\\\\\\\\\\\""+ sum_part +"\\\\\\\\\\\\\"\\\\\\\\n\\\\\\\"\",type=\"" +
+ typ +
----------------
This looks like a magic. \t is a part of the variable's value, so please pass it via arguments too.
BTW: Use add_slashes() from MiLibraryLoadedTestCase.test_lldbmi_library_loaded test case to reduce amount of '\' characters.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:62
@@ -46,4 +61,3 @@
# Test that an char16_t* is expanded to string when print char-array-as-string is "off"
- self.runCmd("-var-create - * u16p")
- self.expect("\^done,name=\"var\d+\",numchild=\"1\",value=\"0x[0-9a-f]+ u\\\\\\\"\\\\\\\\t\\\\\\\\\\\\\"hello\\\\\\\\\\\\\"\\\\\\\\n\\\\\\\"\",type=\"const char16_t \*\",thread-id=\"1\",has_more=\"0\"")
+ self.eval_ap("u16p", "0x[0-9a-f]+ u", "hello", "const char16_t \\*")
----------------
'u' belongs to "hello". Please pass them together.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:85
@@ -74,4 +84,3 @@
# Test that an char[] with escape chars is expanded to string when print char-array-as-string is "on"
- self.runCmd("-var-create - * ca")
- self.expect("\^done,name=\"var\d+\",numchild=\"10\",value=\"\\\\\\\"\\\\\\\\t\\\\\\\\\\\\\"hello\\\\\\\\\\\\\"\\\\\\\\n\\\\\\\"\",type=\"const char \[10\]\",thread-id=\"1\",has_more=\"0\"")
+ self.eval_ap("ca", "", "hello", "const char \\[[0-9]+\\]")
----------------
Why the size is a regexp (it was 10)?
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:100
@@ +99,3 @@
+ # Test russian unicode strings
+ rval = "Эмбаркадеро"
+ self.eval_ap("u16p_rus", "0x[0-9a-f]+ u", rval, "const char16_t \\*")
----------------
Please don't use your company name here.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:109
@@ +108,3 @@
+ # in python regex \\\\ -> \
+ rval = '\\\\' * 7 + '" ' + '\\\\' * 2 + 'n'
+ self.eval_ap("u16p_esc", "0x[0-9a-f]+ u", rval, "const char16_t \\*")
----------------
Where is it from? It must be removed before merging.
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:121-143
@@ -101,3 +120,25 @@
@lldbmi_test
@skipIfWindows #llvm.org/pr24452: Get lldb-mi working on Windows
+ @skipIfFreeBSD # llvm.org/pr22411: Failure presumably due to known thread races
+ @skipIfLinux # llvm.org/pr22841: lldb-mi tests fail on all Linux buildbots
+ def test_lldbmi_stl_types(self):
+ """Test that 'lldb-mi --interpreter' print summary for STL types."""
+
+ self.spawnLldbMi(args = None)
+
+ # Load executable
+ self.runCmd("-file-exec-and-symbols %s" % self.myexe)
+ self.expect("\^done")
+
+ # Run to BP_gdb_set_show_print_char_array_as_string_test
+ line = line_number('main.cpp', '// BP_cpp_stl_types_test')
+ self.runCmd("-break-insert main.cpp:%d" % line)
+ self.expect("\^done,bkpt={number=\"1\"")
+ self.runCmd("-exec-run")
+ self.expect("\^running")
+ self.expect("\*stopped,reason=\"breakpoint-hit\"")
+
+ # Test for std::string
+ self.eval_ap("mi_string", "", "hello", "std::__1::string")
+
----------------
This test case doesn't relate to gdb-set/gdb-show commands, so it should be in test/tools/lldb-mi/variable/TestMiVar.py
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:125
@@ +124,3 @@
+ @skipIfLinux # llvm.org/pr22841: lldb-mi tests fail on all Linux buildbots
+ def test_lldbmi_stl_types(self):
+ """Test that 'lldb-mi --interpreter' print summary for STL types."""
----------------
Rename to something more specific (test_lldbmi_var_create_for_stl_types, for ex)
================
Comment at: test/tools/lldb-mi/variable/TestMiGdbSetShowPrint.py:143
@@ +142,3 @@
+ # Test for std::string
+ self.eval_ap("mi_string", "", "hello", "std::__1::string")
+
----------------
Looks like "std::__1::string" is a platform-dependent name. Don't use it.
================
Comment at: test/tools/lldb-mi/variable/main.cpp:13
@@ -11,1 +12,3 @@
+
+using namespace std;
----------------
Don't use entire namespaces where they aren't needed. Just use std::string on the line #83
================
Comment at: test/tools/lldb-mi/variable/main.cpp:70-73
@@ -66,1 +69,6 @@
+ const char16_t* u16p_rus = u"\t\"Эмбаркадеро\"\n";
+ const char16_t u16a_rus[] = u"\t\"Эмбаркадеро\"\n";
+ const char32_t* u32p_rus = U"\t\"Эмбаркадеро\"\n";
+ const char32_t u32a_rus[] = U"\t\"Эмбаркадеро\"\n";
+
----------------
Please don't use your company name here.
================
Comment at: test/tools/lldb-mi/variable/main.cpp:83
@@ +82,3 @@
+{
+ string mi_string = "\t\"hello\"\n";
+ // BP_cpp_stl_types_test
----------------
just use std::string here, and rename please:
```
std::string std_string = "\t\"hello\"\n";
```
================
Comment at: tools/lldb-mi/MICmnLLDBUtilSBValue.cpp:20-24
@@ -19,2 +19,7 @@
+#if 0
+#include "lldb/Core/ValueObject.h"
+#include "lldb/DataFormatters/TypeSummary.h"
+#endif
+
//++ ------------------------------------------------------------------------------------
----------------
What is it?
http://reviews.llvm.org/D13058
More information about the lldb-commits
mailing list