Skip to content

[API] add GetSyntheticValue #95959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jul 15, 2024
Merged

[API] add GetSyntheticValue #95959

merged 17 commits into from
Jul 15, 2024

Conversation

v-bulle
Copy link
Contributor

@v-bulle v-bulle commented Jun 18, 2024

Adds GetSyntheticValue to the API on top of GetNonSyntheticValue.

@v-bulle v-bulle requested a review from JDevlieghere as a code owner June 18, 2024 17:40
@llvmbot llvmbot added the lldb label Jun 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 18, 2024

@llvm/pr-subscribers-lldb

Author: Vincent Belliard (v-bulle)

Changes

Adds GetSyntheticValue to the API on top of GetNonSyntheticValue.


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

2 Files Affected:

  • (modified) lldb/include/lldb/API/SBValue.h (+2)
  • (modified) lldb/source/API/SBValue.cpp (+12)
diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h
index 65920c76df7a8..bec816fb45184 100644
--- a/lldb/include/lldb/API/SBValue.h
+++ b/lldb/include/lldb/API/SBValue.h
@@ -89,6 +89,8 @@ class LLDB_API SBValue {
 
   lldb::SBValue GetNonSyntheticValue();
 
+  lldb::SBValue GetSyntheticValue();
+
   lldb::DynamicValueType GetPreferDynamicValue();
 
   void SetPreferDynamicValue(lldb::DynamicValueType use_dynamic);
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index 9d7efba024d11..6b77c0e95cedd 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -761,6 +761,18 @@ lldb::SBValue SBValue::GetNonSyntheticValue() {
   return value_sb;
 }
 
+lldb::SBValue SBValue::GetSyntheticValue() {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBValue value_sb;
+  if (IsValid()) {
+    ValueImplSP proxy_sp(new ValueImpl(m_opaque_sp->GetRootSP(),
+                                       m_opaque_sp->GetUseDynamic(), true));
+    value_sb.SetSP(proxy_sp);
+  }
+  return value_sb;
+}
+
 lldb::DynamicValueType SBValue::GetPreferDynamicValue() {
   LLDB_INSTRUMENT_VA(this);
 

@labath labath requested a review from jimingham June 19, 2024 07:45
@labath
Copy link
Collaborator

labath commented Jun 19, 2024

We should also add a test case for the new API. You can just take an existing test case with some synthetic values (e.g. TestFormattersSBAPI.py), add some Get(Non)SyntheticValue calls, and make sure they do what they should.

Copy link

github-actions bot commented Jun 21, 2024

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@@ -155,6 +155,18 @@ def cleanup():
],
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
)
)
self.dbg.GetCategory("CCCSynth").SetEnabled(False)

I'm not sure how lldb chooses the summary providers in case of conflicts, but since that's not what we're testing here, we might as well remove the ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can have a conflict. In this test we have several categories which are enabled one after the other. I just add a new one (with a new name CCCSynth2).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two categories, but they both have a summary provider for the same type (CCC). (Enabling one category does not automatically disable the other ones.) Right now, lldb seems to pick the one you want, but I don't think we promise that anywhere (in fact, I wouldn't be surprised if this comes down to some (nondeterministic) ordering in some hash container). By disabling the first category, we make sure that lldb always pick the one we want (in case, e.g., the container ordering changes). Alternatively, you could just attach the summary provider to a different type (CCC2?)

@jimingham
Copy link
Collaborator

jimingham commented Jun 27, 2024

The one thing about this that strikes me as odd is that if you have an SBValue that doesn't have a Synthetic Value, this will return the non-synthetic value. The GetNonSyntheticValue didn't have this problem because there's always a non-synthetic value, so provided the SBValue was valid you could always hand that out.

Would it make more sense to make the ValueImpl that's going to represent the synthetic value then check whether that really is synthetic, and return an empty SBValue if it is not?

@labath
Copy link
Collaborator

labath commented Jun 27, 2024

Would it make more sense to make the ValueImpl that's going to represent the synthetic value then check whether that really is synthetic, and return an empty SBValue if it is not?

Sounds like a good idea to me.

@v-bulle
Copy link
Contributor Author

v-bulle commented Jul 3, 2024

The one thing about this that strikes me as odd is that if you have an SBValue that doesn't have a Synthetic Value, this will return the non-synthetic value. The GetNonSyntheticValue didn't have this problem because there's always a non-synthetic value, so provided the SBValue was valid you could always hand that out.

Would it make more sense to make the ValueImpl that's going to represent the synthetic value then check whether that really is synthetic, and return an empty SBValue if it is not?

done

Copy link

github-actions bot commented Jul 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@jimingham jimingham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@labath labath merged commit 82af559 into llvm:main Jul 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants