[Lldb-commits] [PATCH] D46128: Fix expression parser to not accept any type whose basename matches for a type that must exist at root level

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

To aid with reviewing I documented LLDB's current behavior. I don't understand the implementation well enough to comment on it, but I do think the the new behavior is superior over the old one. After this patch LLDB behaves deterministic and like the compiler would behave on that line, before it, LLDB would do a DWIM thing with unpredictable results. So unless Jim has any concerns about the implementation, I would give this a go.



================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:48
+        self.assertTrue(expr_result.GetError().Fail(),
+                        "'namespace_only' exists in namespace only")
+
----------------
This currently returns

(a::namespace_only) $0 = (a = 123)

which — while convenient — behaves differently from code injected at that point in the program.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:55
+        expr_result = frame.EvaluateExpression("*((b::namespace_only *)&i)")
+        self.check_value(expr_result, "b", 123)
+
----------------
both of these work at the moment.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:59
+        expr_result = frame.EvaluateExpression("*((namespace_and_file *)&i)")
+        self.check_value(expr_result, "ff", 123)
+        # Make sure we can find the correct type in a namespace "a"
----------------
Currently:

```
error: reference to 'namespace_and_file' is ambiguous
candidate found by name lookup is 'namespace_and_file'
candidate found by name lookup is 'a::namespace_and_file'
error: reference to 'namespace_and_file' is ambiguous
candidate found by name lookup is 'namespace_and_file'
candidate found by name lookup is 'a::namespace_and_file'
```

that's a horrible failure mode.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:67
+                "*((b::namespace_and_file *)&i)")
+        self.check_value(expr_result, "bb", 123)
+
----------------
this works


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:69
+
+        # Make sure we don't accedentally accept structures that exist only
+        # in namespaces when evaluating expressions with top level types.
----------------
accidentally


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:76
+        self.assertTrue(expr_result.GetError().Fail(),
+                        "'in_contains_type' exists in struct only")
+
----------------
(lldb) p *((in_contains_type *)&i)
(a::contains_type::in_contains_type) $5 = (aaa = 123)


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:80
+        expr_result = frame.EvaluateExpression(
+                "*((contains_type::in_contains_type *)&i)")
+        self.check_value(expr_result, "fff", 123)
----------------
Similar ugly failure:

error: reference to 'contains_type' is ambiguous
candidate found by name lookup is 'contains_type'
candidate found by name lookup is 'a::contains_type'
error: expected expression



================
Comment at: packages/Python/lldbsuite/test/lang/cpp/type_lookup/TestTypeLookup.py:88
+        expr_result = frame.EvaluateExpression(
+                "*((b::contains_type::in_contains_type *)&i)")
+        self.check_value(expr_result, "bbb", 123)
----------------
these work as expected.


https://reviews.llvm.org/D46128