[PATCH] D148949: [dataflow] HTMLLogger - show the value of the current expr

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:96
+    for (const auto& Prop : V.properties())
+      JOS.attributeObject(Prop.first(), [&] { dump(*Prop.second); });
+
----------------
mboehme wrote:
> IIUC, this places properties on the current HTML element as attributes, just like the built-in attributes that we add for other purposes (e.g. "value_id", "kind").
> 
>   - What happens if we have a property whose name conflicts with one of the built-in attributes?
>   - Even if we don't have a naming conflict, I think it could be potentially confusing to have user-defined properties appear in the same list and in the same way as built-in attributes.
> 
> Suggestion: Can we nest all properties inside of a "properties" attribute?
> 
> Edit: Having looked at the HTML template now, I see that we exclude certain attributes there ('kind', 'value_id', 'type', 'location') when listing properties. I still think naming conflicts are a potential problem though. I think it would also be clearer to explicitly pick the properties out of a `properties` attribute rather than excluding a blocklist of attributes.
Right, the data model is: a value (really, a Value/StorageLocation mashed together) is just a bag of attributes.

I don't think making it more complicated is an improvement: being built-in isn't the same thing as being custom-rendered.
e.g. "pointee" and "truth" want the default key-value rendering despite being built-in.
Having the exclude list in the template is ugly, but either you end up encoding the rendering info twice in the template like that, or you encode it once in the template and once in the JSON generation (by what goes in the "properties" map vs the main map). I'd rather call this purely a template concern.

Namespace conflict could be a problem: the current behavior is that the last value wins (per JS rules).
IMO the simplest fix is to prepend "p:" and "f:" to properties/struct fields. These would be shown - otherwise the user can't distinguish between a property & field with the same name.

I had this in the prototype, but dropped them because they seemed a bit ugly and conflicts unlikely in practice. WDYT?


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:121-123
+      for (const auto &Child : cast<StructValue>(V).children())
+        JOS.attributeObject(Child.first->getNameAsString(),
+                            [&] { dump(*Child.second); });
----------------
mboehme wrote:
> 
this is neat but capturing the structured binding `Val` is a C++20 feature


================
Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:149
+    case Value::Kind::Biconditional: {
+      auto &VV = cast<ImplicationValue>(V);
+      JOS.attributeObject("lhs", [&] { dump(VV.getLeftSubValue()); });
----------------
mboehme wrote:
> 
right, thanks :-(
Can't wait to get rid of this stuff :-)


================
Comment at: clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp:179
   EXPECT_THAT(Logs[0], HasSubstr("LocToVal")) << "has built-in lattice dump";
+  EXPECT_THAT(Logs[0], HasSubstr("\"type\": \"int\"")) << "has value dump";
 }
----------------
mboehme wrote:
> Can you add some additional tests for the following?
> 
>   - Pointers
>   - References
>   - Properties
>   - Struct fields
> 
> Ideally, other value kinds too (see copy-paste error above for `BiconditionalValue`).
There isn't a reasonable way to test in that level of detail without paying a lot for it. 
String assertions on the HTML are not suitable for testing nontrivial structures in the JSON.

If we restructure to allow such assertions on the JSON:
 - it would be significantly expensive/intrusive
 - we would be relying on details of the analysis itself and setting them up indirectly - quite brittle
 - we would spend a lot on testing and still not look at all at what matters (the rendering)

My experience is that not having proper tests for these kind of tools is the least-worst option: better than brittle tests where true vs spurious errors are unclear, and better than expensive+still incomplete tests.
The debugging tools tend to get changed infrequently, manually verified, and sometimes an annoying regression creeps in.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148949/new/

https://reviews.llvm.org/D148949