[FlowSensitive] Allow to dump nested RecordStorageLocation

[fmayer]

We have an internal analysis that uses them, and the HTML dump would
fail on the assertion.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

Author: Florian Mayer (fmayer)

Changes

We have an internal analysis that uses them, and the HTML dump would
fail on the assertion.

---

Full diff: https://github.com/llvm/llvm-project/pull/112457.diff

1 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp (+1-2)

diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index a36cb41a63dfb1..557df218837941 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -133,8 +133,7 @@ class ModelDumper {
       for (const auto &Child : RLoc->children())
         JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
           if (Child.second)
-            if (Value *Val = Env.getValue(*Child.second))
-              dump(*Val);
+            dump(*Child.second);
         });
 
       for (const auto &SyntheticField : RLoc->synthetic_fields())

[llvmbot]

@llvm/pr-subscribers-clang

Author: Florian Mayer (fmayer)

Changes

We have an internal analysis that uses them, and the HTML dump would
fail on the assertion.

---

Full diff: https://github.com/llvm/llvm-project/pull/112457.diff

1 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp (+1-2)

diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
index a36cb41a63dfb1..557df218837941 100644
--- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
+++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp
@@ -133,8 +133,7 @@ class ModelDumper {
       for (const auto &Child : RLoc->children())
         JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] {
           if (Child.second)
-            if (Value *Val = Env.getValue(*Child.second))
-              dump(*Val);
+            dump(*Child.second);
         });
 
       for (const auto &SyntheticField : RLoc->synthetic_fields())

[fmayer]

An example dump that is now possible but crashed before

[ymand]

I would suggest a brief comment explaining the choice not to filter.

[fmayer]

I would suggest a brief comment explaining the choice not to filter.

I'm not sure I understand. There wasn't a choice to filter before, there was just the (incorrect) assumption that we don't have nested RecordStorageLocation, leading to a crash.

[martinboehme]

I think this is the right change.

What I don't understand, though, is why you were getting an assert failure before. (Which line is the assertion on that failed?) I would have thought if you don't dump the nested record, you just get less information. Apparently not so?

I would suggest a brief comment explaining the choice not to filter.

I'm not sure I understand. There wasn't a choice to filter before, there was just the (incorrect) assumption that we don't have nested RecordStorageLocation, leading to a crash.

@ymand I think maybe what you're missing is that the code is calling a different overload of dump() now? Previously, it called the Value overload, now it's calling the StorageLocation overload. And of course the StorageLocation doesn't care whether there's a value at the location (or rather, it will figure that out itself and dump the value if so).

@fmayer Just as an explanation of why this check is there: I think this isn't because the code assumed that nested RecordStorageLocations don't exist; rather, we used to have a concept of a RecordValue, and when that existed, it made more sense to do this check here. When I eliminated the RecordValue concept, I updated some of the code in ModelDumper but didn't notice that this check now no longer made sense.

[fmayer]

I think this is the right change.

What I don't understand, though, is why you were getting an assert failure before. (Which line is the assertion on that failed?) I would have thought if you don't dump the nested record, you just get less information. Apparently not so?

Because the first line of Env.getValue is

  assert(!isa<RecordStorageLocation>(Loc));

[martinboehme]

I think this is the right change.
What I don't understand, though, is why you were getting an assert failure before. (Which line is the assertion on that failed?) I would have thought if you don't dump the nested record, you just get less information. Apparently not so?

Because the first line of Env.getValue is

  assert(!isa<RecordStorageLocation>(Loc));

Ah, of course, that makes sense. Thanks!