[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks for the notice. I apologize for not doing everything correctly. I'm 
still trying to understand how all the different piece of the system works and 
I end up learning from the things that fail...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

commit fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 should be 
associated with `Differential Revision: https://reviews.llvm.org/D76111`.

The revert of fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 should 
explain why fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 
 was 
reverted.

Please don't make a commit whose description contains just one word `fix`.

If you are unsure, upload a new diff to trigger Harbormaster, check its test 
status on 3 Linux/Windows/macOS, instead of committing and reverting like 
playing a game. Every commit has a permanent record which will be shared among 
thousands of developers and can increase the size of the repository. Please be 
careful.

`Subscribers:` and `Tags:` lines are generally considered useless. You can 
strip them with:

  arcfilter () {
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: 
/,"");print}' | git commit --amend -F -
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

It looks like this test doesn't work on the bots: 
http://green.lab.llvm.org/green//view/LLDB/job/lldb-cmake/lastCompletedBuild//testReport/junit/lldb-api/python_api_sbenvironment/TestSBEnvironment_py/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Since it's late on Friday afternoon I'm taking the liberty to revert the patch 
to get the bots running again. Feel free to re-land with a fix or ask for 
another round of review if you have more questions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Phabricator via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2dec82652e4b: Create basic SBEnvironment class (authored by 
Walter Erquinigo walterme...@fb.com).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBLaunchInfo.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBLaunchInfo.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,125 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+# We use this function to test both kind of accessors:
+# .  Get*AtIndex and GetEntries
+def assertEqualEntries(self, env, entries):
+self.assertEqual(env.GetNumValues(), len(entries))
+for i in range(env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+self.assertIn(name + "=" + value, entries)
+
+entries = env.GetEntries()
+self.assertEqual(entries.GetSize(), len(entries))
+for i in range(entries.GetSize()):
+(name, value) = entries.GetStringAtIndex(i).split("=")
+self.assertIn(name + "=" + value, entries)
+
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_launch_info(self):
+target = self.dbg.CreateTarget("")
+launch_info = target.GetLaunchInfo()
+env = launch_info.GetEnvironment()
+self.assertEqual(env.GetNumValues(), 0)
+
+env.Set("FOO", "bar", overwrite=True)
+self.assertEqual(env.GetNumValues(), 1)
+
+# Make sure we only modify the copy of the launchInfo's environment
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 0)
+
+launch_info.SetEnvironment(env, append=True)
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 1)
+
+# Make sure we can replace the launchInfo's environment
+env.Clear()
+env.Set("BAR", "foo", overwrite=True)
+env.PutEntry("X=y")
+launch_info.SetEnvironment(env, append=False)
+self.assertEqualEntries(launch_info.GetEnvironment(), ["BAR=foo", "X=y"])
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.PutEntry("PATH=#" + path)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+self.assertEqualEntries(env, ["FOO=", "BAR=foo"])
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+self.assertEqual(env.Get("FOO"), 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251740.
wallace added a comment.

apply last suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBLaunchInfo.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBLaunchInfo.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,125 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+# We use this function to test both kind of accessors:
+# .  Get*AtIndex and GetEntries
+def assertEqualEntries(self, env, entries):
+self.assertEqual(env.GetNumValues(), len(entries))
+for i in range(env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+self.assertIn(name + "=" + value, entries)
+
+entries = env.GetEntries()
+self.assertEqual(entries.GetSize(), len(entries))
+for i in range(entries.GetSize()):
+(name, value) = entries.GetStringAtIndex(i).split("=")
+self.assertIn(name + "=" + value, entries)
+
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_launch_info(self):
+target = self.dbg.CreateTarget("")
+launch_info = target.GetLaunchInfo()
+env = launch_info.GetEnvironment()
+self.assertEqual(env.GetNumValues(), 0)
+
+env.Set("FOO", "bar", overwrite=True)
+self.assertEqual(env.GetNumValues(), 1)
+
+# Make sure we only modify the copy of the launchInfo's environment
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 0)
+
+launch_info.SetEnvironment(env, append=True)
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 1)
+
+# Make sure we can replace the launchInfo's environment
+env.Clear()
+env.Set("BAR", "foo", overwrite=True)
+env.PutEntry("X=y")
+launch_info.SetEnvironment(env, append=False)
+self.assertEqualEntries(launch_info.GetEnvironment(), ["BAR=foo", "X=y"])
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.PutEntry("PATH=#" + path)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+self.assertEqualEntries(env, ["FOO=", "BAR=foo"])
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+self.assertEqual(env.Get("FOO"), "")
+
+env.PutEntry("FOO=bar")
+self.assertEqual(env.Get("FOO"), "bar")
+
+self.assertEqualEntries(env, ["FOO=bar", "BAR=foo"])
+
+# Make sure we 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks a lot, guys. I learned a lot from you doing this patch :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

This looks good to me. I still have some doubts about the ConstStringification 
of the returned values (I am not a fan of constifying everything), but I don't 
want to block this review for that.




Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+  ConstString(std::next(m_opaque_up->begin(), index)->first())
+  .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+ index);

wallace wrote:
> clayborg wrote:
> > labath wrote:
> > > I don't think these need to const-stringified now, given that they are 
> > > backed by the underlying map. We already have functions which return 
> > > pointers to internal objects (e.g. SBStream::GetData).
> > > 
> > > @clayborg? 
> > That's a tough one. I would like to think that any "const char *" that 
> > comes back from an API that returns strings could just be pointer compared 
> > if needed. So I like the idea that for strings that comes out of the API 
> > that they are all const-ed and could easily be compared. I am fine with 
> > SBStream::GetData() returning its own string because _it_ owns the data. So 
> > I would vote to ConstString() any returned results
> I'm adding a Clear method. With that, the backing char* might be wiped out 
> and the python reference to this string might be invalid unless we use 
> ConstString
> I am fine with SBStream::GetData() returning its own string because _it_ owns 
> the data.
I am afraid I don't see the distinction here. SBStream owns a Stream (via a 
unique_ptr) and the stream data is inside that. An SBEnvironment has a 
unique_ptr, and the strings are inside of that.

> I'm adding a Clear method. With that, the backing char* might be wiped out 
> and the python reference to this string might be invalid unless we use 
> ConstString
Python will be fine if as it will copy the string as soon as it gets its hand 
on it. C++ users could be affected by that, but, this is not something that 
should come across as surprising to them (and indeed, the same thing will 
happen with SBStream::GetData).



Comment at: lldb/source/API/SBEnvironment.cpp:26
+
+  m_opaque_up = clone(rhs.m_opaque_up);
+}

move this into the initializer list?



Comment at: lldb/source/API/SBEnvironment.cpp:29
+
+SBEnvironment::SBEnvironment(const Environment )
+: m_opaque_up(new Environment(rhs)) {}

super-nit: if you make this constructor accept a `Environment rhs` (without the 
`const &` part) and then do the initialization as `new 
Environment(std::move(rhs))`, you will avoid copying the environment in the 
expressions like `SBEnvironment(Environment(whatever))`. Currently that will 
create one `Environment` object to initialize the `rhs` argument, and then a 
second one when copying things to `m_opaque_up`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251472.
wallace added a comment.

fix grammar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBLaunchInfo.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBLaunchInfo.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,125 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+# We use this function to test both kind of accessors:
+# .  Get*AtIndex and GetEntries
+def assertEqualEntries(self, env, entries):
+self.assertEqual(env.GetNumValues(), len(entries))
+for i in range(env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+self.assertIn(name + "=" + value, entries)
+
+entries = env.GetEntries()
+self.assertEqual(entries.GetSize(), len(entries))
+for i in range(entries.GetSize()):
+(name, value) = entries.GetStringAtIndex(i).split("=")
+self.assertIn(name + "=" + value, entries)
+
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_launch_info(self):
+target = self.dbg.CreateTarget("")
+launch_info = target.GetLaunchInfo()
+env = launch_info.GetEnvironment()
+self.assertEqual(env.GetNumValues(), 0)
+
+env.Set("FOO", "bar", overwrite=True)
+self.assertEqual(env.GetNumValues(), 1)
+
+# Make sure we only modify the copy of the launchInfo's environment
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 0)
+
+launch_info.SetEnvironment(env, append=True)
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 1)
+
+# Make sure we can replace the launchInfo's environment
+env.Clear()
+env.Set("BAR", "foo", overwrite=True)
+env.PutEntry("X=y")
+launch_info.SetEnvironment(env, append=False)
+self.assertEqualEntries(launch_info.GetEnvironment(), ["BAR=foo", "X=y"])
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.PutEntry("PATH=#" + path)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+self.assertEqualEntries(env, ["FOO=", "BAR=foo"])
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+self.assertEqual(env.Get("FOO"), "")
+
+env.PutEntry("FOO=bar")
+self.assertEqual(env.Get("FOO"), "bar")
+
+self.assertEqualEntries(env, ["FOO=bar", "BAR=foo"])
+
+# Make sure we can unset
+   

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251470.
wallace added a comment.

- Added both kinds of APIs we were discussing. It will come handy for all 
different kind of usages
- Added an SBEnvironment API for SBLaunchInfo, which will be used in 
https://reviews.llvm.org/D74636
- Address all sorts of comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBLaunchInfo.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBLaunchInfo.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBLaunchInfo.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,125 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+# We use this function to test both kind of accessors:
+# .  Get*AtIndex and GetEntries
+def assertEqualEntries(self, env, entries):
+self.assertEqual(env.GetNumValues(), len(entries))
+for i in range(env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+self.assertIn(name + "=" + value, entries)
+
+entries = env.GetEntries()
+self.assertEqual(entries.GetSize(), len(entries))
+for i in range(entries.GetSize()):
+(name, value) = entries.GetStringAtIndex(i).split("=")
+self.assertIn(name + "=" + value, entries)
+
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_launch_info(self):
+target = self.dbg.CreateTarget("")
+launch_info = target.GetLaunchInfo()
+env = launch_info.GetEnvironment()
+self.assertEqual(env.GetNumValues(), 0)
+
+env.Set("FOO", "bar", overwrite=True)
+self.assertEqual(env.GetNumValues(), 1)
+
+# Make sure we only modify the copy of the launchInfo's environment
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 0)
+
+launch_info.SetEnvironment(env, append=True)
+self.assertEqual(launch_info.GetEnvironment().GetNumValues(), 1)
+
+# Make sure we can replace the launchInfo's environment
+env.Clear()
+env.Set("BAR", "foo", overwrite=True)
+env.PutEntry("X=y")
+launch_info.SetEnvironment(env, append=False)
+self.assertEqualEntries(launch_info.GetEnvironment(), ["BAR=foo", "X=y"])
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.PutEntry("PATH=#" + path)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+self.assertEqualEntries(env, ["FOO=", "BAR=foo"])
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 2 inline comments as done.
wallace added inline comments.



Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+  ConstString(std::next(m_opaque_up->begin(), index)->first())
+  .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+ index);

clayborg wrote:
> labath wrote:
> > I don't think these need to const-stringified now, given that they are 
> > backed by the underlying map. We already have functions which return 
> > pointers to internal objects (e.g. SBStream::GetData).
> > 
> > @clayborg? 
> That's a tough one. I would like to think that any "const char *" that comes 
> back from an API that returns strings could just be pointer compared if 
> needed. So I like the idea that for strings that comes out of the API that 
> they are all const-ed and could easily be compared. I am fine with 
> SBStream::GetData() returning its own string because _it_ owns the data. So I 
> would vote to ConstString() any returned results
I'm adding a Clear method. With that, the backing char* might be wiped out and 
the python reference to this string might be invalid unless we use ConstString



Comment at: lldb/source/API/SBEnvironment.cpp:80-81
+ overwrite);
+  if (overwrite || m_opaque_up->find(name) == m_opaque_up->end()) {
+m_opaque_up->insert_or_assign(name, std::string(value));
+return LLDB_RECORD_RESULT(true);

labath wrote:
> how about `if(overwrite) insert_or_assign(name, value) else try_emplace(name, 
> value)`? (avoiding two map lookups)
teach me more


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D76111#1930783 , @labath wrote:

> The new interface is ok for me, but there are two things I want to mention:
>
> - the iteration via `Get{Name,Value}AtIndex` is going to be slow because the 
> underlying map (like most maps) does not have random access iterators. This 
> is the problem I was trying to solve with the `GetEntries`-like API, but I 
> agree that has its issues too.


I am guessing people won't use these much. I didn't know he underlying class 
used a map. No great solution here unless we modify the underlying class to 
have a vector with a map of name to index?

> - I agree that forcing a user to manually construct `name=value` strings if 
> he has then as separate entities is not good, but I also don't think we're 
> doing anyone a favor by not providing an api which would accept such strings. 
> The `name=value` format is very universal, and I think users will often have 
> the value in this format already (for example, D74636 
>  does). This means that those users would 
> have to implement `=`-splitting themselves before using this class. This is 
> why the internal Environment class provides both kinds of api, and maybe also 
> the reason why the c library provides both `putenv(3)` and `setenv(3)`.

Fine to add functions for this. The user might grab the environment dump in 
text and want to set all of those env vars, so having the API is fine with me.




Comment at: lldb/source/API/SBEnvironment.cpp:59
+ index);
+  if (index < 0 || index >= GetNumValues())
+return LLDB_RECORD_RESULT(nullptr);

labath wrote:
> `index < 0` is not possible now.
yeah, size_t is unsigned on all platforms.



Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+  ConstString(std::next(m_opaque_up->begin(), index)->first())
+  .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+ index);

labath wrote:
> I don't think these need to const-stringified now, given that they are backed 
> by the underlying map. We already have functions which return pointers to 
> internal objects (e.g. SBStream::GetData).
> 
> @clayborg? 
That's a tough one. I would like to think that any "const char *" that comes 
back from an API that returns strings could just be pointer compared if needed. 
So I like the idea that for strings that comes out of the API that they are all 
const-ed and could easily be compared. I am fine with SBStream::GetData() 
returning its own string because _it_ owns the data. So I would vote to 
ConstString() any returned results



Comment at: lldb/source/API/SBEnvironment.cpp:81
+  if (overwrite || m_opaque_up->find(name) == m_opaque_up->end()) {
+m_opaque_up->insert_or_assign(name, std::string(value));
+return LLDB_RECORD_RESULT(true);

Or do we change the underlying API in the m_opaque_up class to accept an 
overwrite arg?



Comment at: lldb/source/API/SBPlatform.cpp:658-660
+  if (platform_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(platform_sp->GetEnvironment()));
+  }

remove {} (clang code guidelines for single statement if)



Comment at: lldb/source/API/SBTarget.cpp:2396-2398
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }

remove {} (clang code guidelines for single statement if)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Well, I think I'll implement both kind of accessors in the API to account for 
all possible cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The new interface is ok for me, but there are two things I want to mention:

- the iteration via `Get{Name,Value}AtIndex` is going to be slow because the 
underlying map (like most maps) does not have random access iterators. This is 
the problem I was trying to solve with the `GetEntries`-like API, but I agree 
that has its issues too.
- I agree that forcing a user to manually construct `name=value` strings if he 
has then as separate entities is not good, but I also don't think we're doing 
anyone a favor by not providing an api which would accept such strings. The 
`name=value` format is very universal, and I think users will often have the 
value in this format already (for example, D74636 
 does). This means that those users would have 
to implement `=`-splitting themselves before using this class. This is why the 
internal Environment class provides both kinds of api, and maybe also the 
reason why the c library provides both `putenv(3)` and `setenv(3)`.




Comment at: lldb/source/API/SBEnvironment.cpp:53
+  return LLDB_RECORD_RESULT(
+  ConstString(m_opaque_up->lookup(name)).AsCString(""));
+}

Please reuse the result of `m_opaque_up->find(name)` here to avoid double 
lookup.



Comment at: lldb/source/API/SBEnvironment.cpp:59
+ index);
+  if (index < 0 || index >= GetNumValues())
+return LLDB_RECORD_RESULT(nullptr);

`index < 0` is not possible now.



Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+  ConstString(std::next(m_opaque_up->begin(), index)->first())
+  .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+ index);

I don't think these need to const-stringified now, given that they are backed 
by the underlying map. We already have functions which return pointers to 
internal objects (e.g. SBStream::GetData).

@clayborg? 



Comment at: lldb/source/API/SBEnvironment.cpp:80-81
+ overwrite);
+  if (overwrite || m_opaque_up->find(name) == m_opaque_up->end()) {
+m_opaque_up->insert_or_assign(name, std::string(value));
+return LLDB_RECORD_RESULT(true);

how about `if(overwrite) insert_or_assign(name, value) else try_emplace(name, 
value)`? (avoiding two map lookups)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just some header doc cleanup before we submit. Thanks for sticking with these 
changes!




Comment at: lldb/include/lldb/API/SBEnvironment.h:24
+
+  const lldb::SBEnvironment =(const lldb::SBEnvironment );
+

const means nothing in these classes since the ivar will never get modified, so 
you can remote the leading "const" for the return value. 



Comment at: lldb/include/lldb/API/SBEnvironment.h:27
+  /// Return the value of a given environment variable.
+  ///
+  /// \return

need \param entries in header doc



Comment at: lldb/include/lldb/API/SBEnvironment.h:29
+  /// \return
+  /// The value of the enviroment variable or null if not present.
+  const char *Get(const char *name);

Rephrase the return value docs a bit. Maybe:

```
/// The value of the environment variable or null if not present. If the 
environment variable has no value but is present a valid pointer to an empty 
string will be returned.
```

Is that what we are doing? Returning "" when variable is there but has no value?



Comment at: lldb/include/lldb/API/SBEnvironment.h:32
+
+  /// \return
+  /// The name at the given index or null if the index is invalid.

need brief description and \param entries in header doc



Comment at: lldb/include/lldb/API/SBEnvironment.h:36
+
+  /// \return
+  /// The value at the given index or null if the index is invalid.

need brief description and \param entries in header doc



Comment at: lldb/include/lldb/API/SBEnvironment.h:37
+  /// \return
+  /// The value at the given index or null if the index is invalid.
+  const char *GetValueAtIndex(size_t index);

Need to clarify what happens when the env far is there but has no value. Should 
match the Get(const char*) function return value with what ever we say is the 
solution for that.



Comment at: lldb/include/lldb/API/SBEnvironment.h:40
+
+  size_t GetNumValues();
+

Move above the *AtIndex(size_t) calls so users see this first in API.



Comment at: lldb/include/lldb/API/SBEnvironment.h:44
+  /// If the variable exists, its value is updated only if overwrite is true.
+  ///
+  /// \return

need \param entries in header doc



Comment at: lldb/include/lldb/API/SBEnvironment.h:50
+  /// Unset an environment variable if exists.
+  ///
+  /// \return

need \param entries in header doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251201.
wallace added a comment.

nit 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,86 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+def forEachEntry(env, callback):
+for i in range(0, env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+callback(name, value)
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.Set("PATH", "#" + path, overwrite=True)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+forEachEntry(
+env, 
+lambda name, value:
+   self.assertIn(name + '=' + value, ["FOO=", "BAR=foo"])
+)
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+self.assertEqual(env.Get("FOO"), "")
+
+self.assertTrue(env.Set("FOO", "bar", overwrite=True))
+self.assertEqual(env.Get("FOO"), "bar")
+
+forEachEntry(
+env, 
+lambda name, value:
+   self.assertIn(name + '=' + value, ["FOO=bar", "BAR=foo"])
+)
+
+# Make sure we can unset
+self.assertTrue(env.Unset("FOO"))
+self.assertFalse(env.Unset("FOO"))
+self.assertEqual(env.Get("FOO"), None)
\ No newline at end of file
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -2643,6 +2655,7 @@
   LLDB_REGISTER_METHOD(lldb::SBInstructionList, SBTarget,
GetInstructionsWithFlavor,
(lldb::addr_t, 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251200.
wallace added a comment.

Addressed Greg's comments.

At the end I ended up not using APIs that use the format NAME=value, as they 
could be error prone. I think it's safer to use functions that work with 
explicit names and values.

Besides, I don't want to complicate this diff, so for now I'd prefer not to add 
setters for Platform and Target using the Environment class. We can add that 
later when we need it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,86 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+def forEachEntry(env, callback):
+for i in range(0, env.GetNumValues()):
+name = env.GetNameAtIndex(i)
+value = env.GetValueAtIndex(i)
+callback(name, value)
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.Get("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(env.GetNumValues(), 0)
+self.assertEqual(env.Get("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.Get("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.Set("PATH", "#" + path, overwrite=True)
+self.assertEqual(target.GetEnvironment().Get("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.Get("FOO"), None)
+self.assertEqual(env.Get("BAR"), None)
+
+# We also test empty values
+self.assertTrue(env.Set("FOO", "", overwrite=False))
+env.Set("BAR", "foo", overwrite=False)
+
+self.assertEqual(env.Get("FOO"), "")
+self.assertEqual(env.Get("BAR"), "foo")
+
+self.assertEqual(env.GetNumValues(), 2)
+
+forEachEntry(
+env, 
+lambda name, value:
+   self.assertIn(name + '=' + value, ["FOO=", "BAR=foo"])
+)
+
+# Make sure modifications work
+self.assertFalse(env.Set("FOO", "bar", overwrite=False))
+self.assertEqual(env.Get("FOO"), "")
+
+self.assertTrue(env.Set("FOO", "bar", overwrite=True))
+self.assertEqual(env.Get("FOO"), "bar")
+
+forEachEntry(
+env, 
+lambda name, value:
+   self.assertIn(name + '=' + value, ["FOO=bar", "BAR=foo"])
+)
+
+# Make sure we can unset
+self.assertTrue(env.Unset("FOO"))
+self.assertFalse(env.Unset("FOO"))
+self.assertEqual(env.Get("FOO"), None)
\ No newline at end of file
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

We need to determine if the objects we return are copies (from SBPlatform and 
SBTarget), or if they are objects that will modify the environment for the 
platform and target respectively. If we are returning copies, we need setters 
on both the platform and target. If they are references, then they will just 
update the current environment. I think I would rather have the ability to 
modify them using the returned object, but I am not set on this if everyone 
else thinks otherwise.

I added many comments in the SBEnvironment header.




Comment at: lldb/include/lldb/API/SBEnvironment.h:26
+
+  size_t GetNumEntries();
+

would GetNumValues() be better? Also, if we have this, we need some accessors 
that work by index:

```
const char *GetNameAtIndex(size_t);
const char *GetValueAtIndex(size_t);
```

Again, I would rather people not have to split up the name and value themselves 
if we can avoid it.



Comment at: lldb/include/lldb/API/SBEnvironment.h:32
+  /// The value of the enviroment variable or null if not present.
+  const char *GetEntry(const char *name);
+

Would get Get(...) be better? Mirror the getenv() call?



Comment at: lldb/include/lldb/API/SBEnvironment.h:39
+  /// The value of the environment variable or null if not present.
+  lldb::SBStringList GetEntries();
+

If we can get a list of values and edit the SBStringList, do we want a setter?:

```
void SetEntries(SBStringList);
```



Comment at: lldb/include/lldb/API/SBEnvironment.h:46
+  /// \return
+  void PutEntry(const char *nameAndValue);
+

labath wrote:
> we use the `name_and_value` style elsewhere
Change to:

```
bool Set(const char *name, const char *value, bool overwrite);
```

The bool indicates if it was set. I would rather people not have to make string 
values that contain "name=value" just to set an env var. This mirrors the 
setenv() API in libc






Comment at: lldb/include/lldb/API/SBEnvironment.h:47
+  void PutEntry(const char *nameAndValue);
+
+protected:

We need a Unset accessor:

```
bool Unset(const char *name);
```

The returned bool can indicate if the value was removed.



Comment at: lldb/include/lldb/API/SBTarget.h:97
 
+  /// Return the environment variables or the target.
+  ///

labath wrote:
> s/or/of
Maybe better as:

```
/// Return an object that contains the environment that will be used when 
/// launching processes.
```

Then we need to explain if this is a copy, or a value that can be modified. It 
is can be modified, we should add

```
/// The SBEnvironment object can be used to add or remove environment 
/// variables prior to launching a process.
```

If it is a copy, mention using the SetEnvironment accessor I mention below




Comment at: lldb/include/lldb/API/SBTarget.h:100
+  /// \return
+  /// An lldb::SBEnvironment object with the taget's environment variables.
+

labath wrote:
> s/taget/target/
> 
> Also the concept of a "target's environment variables" is somewhat fuzzy. I 
> take it this is the environment that would be used by default to launch a new 
> process (aka the value of target.env-vars setting)?
> 
> And maybe also mention that this is a *copy* of that environment (so any 
> changes made to it don't propagate back anywhere)
If this is a copy of the environment, then there should be a set accessor:

```
void SBTarget::SetEnvironment(SBEnvironment);
```

I would be ok with either way (return a copy and have setting, or return the 
actual object so it can be manipulated). What is the best approach?



Comment at: lldb/source/API/SBEnvironment.cpp:52
+ name);
+  return 
LLDB_RECORD_RESULT(ConstString(m_opaque_up->lookup(name)).AsCString());
+}

labath wrote:
> Can you check what will this return for non-existing variables and for 
> variables which are set to an empty string `FOO=`. I have a feeling this will 
> return the same thing, but I would expect to get a nullptr in the first case, 
> and `""` in the second.
Yes, we need a test for env vars with that are set with no value and it should 
return "" (non NULL).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 251143.
wallace added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Utility/Environment.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py

Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,67 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertNotEqual(env.GetEntry("PATH"), None)
+
+
+@add_test_categories(['pyapi'])
+def test_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(len(env.GetEntries()), 0)
+self.assertEqual(env.GetEntry("PATH"), None)
+
+target = self.dbg.CreateTarget("")
+env = target.GetEnvironment()
+path = env.GetEntry("PATH")
+# Now there's a target, so at least PATH should exist
+self.assertNotEqual(path, None)
+
+# Make sure we are getting a copy by modifying the env we just got
+env.PutEntry("PATH=#" + path)
+self.assertEqual(target.GetEnvironment().GetEntry("PATH"), path)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.GetEntry("FOO"), None)
+self.assertEqual(env.GetEntry("BAR"), None)
+
+# We also test empty values
+env.PutEntry("FOO=")
+env.PutEntry("BAR=foo")
+
+self.assertEqual(env.GetEntry("FOO"), "")
+self.assertEqual(env.GetEntry("BAR"), "foo")
+
+entries = env.GetEntries()
+self.assertEqual(len(entries), 2)
+
+for entry in env.GetEntries():
+self.assertIn(entry, ["FOO=", "BAR=foo"])
+
+# Make sure modifications work
+env.PutEntry("FOO=bar")
+self.assertEqual(env.GetEntry("FOO"), "bar")
+
+for entry in env.GetEntries():
+self.assertIn(entry, ["FOO=bar", "BAR=foo"])
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -2643,6 +2655,7 @@
   LLDB_REGISTER_METHOD(lldb::SBInstructionList, SBTarget,
GetInstructionsWithFlavor,
(lldb::addr_t, const char *, const void *, size_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBTarget, GetEnvironment, ());
 }
 
 }
Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 #include "lldb/API/SBUnixSignals.h"
 #include "lldb/Host/File.h"
 #include "lldb/Target/Platform.h"
@@ -649,6 +651,17 @@
   return LLDB_RECORD_RESULT(SBUnixSignals());
 }
 

Re: [Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via lldb-commits]

[Splitting this off to not derail the review]

On 14/03/2020 00:45, Greg Clayton via Phabricator wrote:
> labath wrote:
>> This will return a dangling pointer as Envp is a temporary object. It's 
>> intended to be used to pass the environment object to execve-like function. 
>> The current "state of the art" is to ConstStringify all temporary strings 
>> that go out of the SB api. (Though I think we should start using 
>> std::string).
>>
>> Also, constructing the Envp object just to fetch a single string out of it 
>> is pretty expensive. You can fetch the desired result from the Environment 
>> object directly.
>>
>> Putting it all together, this kind of an API would be implemented via 
>> something like:
>> ```
>>   if (idx >= m_opaque_up->size())
>> return nullptr;
>>   return ConstString(Environment::compose(*std::next(m_opaque_up.begin(), 
>> idx)).AsCString();
>> ```
>>
>> ... but I am not sure this is really the right api. More on that in a 
>> separate comment.
> Yes, any C strings returned from the SB API should use 
> ConstString(cstr).AsCString() as the ConstString puts the string into a 
> string pool and there is no need to clients to destruct the string or take 
> ownership.
> 
> No STL in the SB API please, so no std::string. std::string isn't stable on 
> all platforms and competing C++ runtimes on different platforms can cause 
> problems. If we need string ownership, we can introduce SBString as a class.

What do you exactly mean by "not stable"? All major C++ standard
libraries I have knowledge of (libstdc++, libc++, msvc stl) take abi
stability very seriously (more seriously than we do for SB api).

I can see how returning std::strings would be a problem if one builds
liblldb with one c++ library (say libstdc++) and then tries to build the
dependant program with libc++ (are those the competing runtimes you
mentioned?), but.. is that something we have signed up to support?

The main reason I am even thinking about this is because swig already
knows how to map common stl types onto native types of other languages.
This is particularly important if we are going to have multiple
scripting languages, as we wouldn't need to redo the mapping for each
script language and each stl-like class we have (e.g. all SBXXXLists)

pl
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks pretty good overall. I have a bunch of comments, but nothing major.

Could you also please rebase the other patch (D74636 
) on top of this. I think that a very common 
use case for this will be taking the platform environment, tweaking it, and 
then using it to launch a process (i.e. what you are about to do), so it would 
be good to make sure that flow is sufficiently straight-forward.




Comment at: lldb/include/lldb/API/SBEnvironment.h:38
+  /// \return
+  /// The value of the environment variable or null if not present.
+  lldb::SBStringList GetEntries();

This doesn't sounds right.



Comment at: lldb/include/lldb/API/SBEnvironment.h:41-43
+  /// Adds an environment variable to this object. The input must be a string
+  /// with the format
+  ///   VARIABLE=value

Maybe explicitly mention that this overwrites any previous value with that name?



Comment at: lldb/include/lldb/API/SBEnvironment.h:46
+  /// \return
+  void PutEntry(const char *nameAndValue);
+

we use the `name_and_value` style elsewhere



Comment at: lldb/include/lldb/API/SBTarget.h:97
 
+  /// Return the environment variables or the target.
+  ///

s/or/of



Comment at: lldb/include/lldb/API/SBTarget.h:100
+  /// \return
+  /// An lldb::SBEnvironment object with the taget's environment variables.
+

s/taget/target/

Also the concept of a "target's environment variables" is somewhat fuzzy. I 
take it this is the environment that would be used by default to launch a new 
process (aka the value of target.env-vars setting)?

And maybe also mention that this is a *copy* of that environment (so any 
changes made to it don't propagate back anywhere)



Comment at: lldb/source/API/SBEnvironment.cpp:52
+ name);
+  return 
LLDB_RECORD_RESULT(ConstString(m_opaque_up->lookup(name)).AsCString());
+}

Can you check what will this return for non-existing variables and for 
variables which are set to an empty string `FOO=`. I have a feeling this will 
return the same thing, but I would expect to get a nullptr in the first case, 
and `""` in the second.



Comment at: lldb/source/API/SBEnvironment.cpp:57
+  LLDB_RECORD_METHOD_NO_ARGS(SBStringList, SBEnvironment, GetEntries);
+  auto entries = SBStringList();
+  for (const auto  : *m_opaque_up) {

This is using `auto` just to be fancy. That is not consistent with [[ 
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
 | llvm style ]].



Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:31-32
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), 
CURRENT_EXECUTABLE_SET)
+env = self.dbg.GetSelectedTarget().GetEnvironment()

I don't think you actually need to build an inferior for this. `target = 
dbg.CreateTarget("")` should be sufficient, I believe.



Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:45
+env.PutEntry("FOO=bar")
+env.PutEntry("BAR=foo")
+

As per the other comment, please also test a case like `env.PutEntry("BAZ=")`



Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:54
+for entry in env.GetEntries():
+self.assertTrue(entry in ["FOO=bar", "BAR=foo"])

self.assertIn(needle, haystack)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 250649.
wallace added a comment.

address comments≈


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/Makefile
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
  lldb/test/API/python_api/sbenvironment/main.cpp

Index: lldb/test/API/python_api/sbenvironment/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/main.cpp
@@ -0,0 +1,4 @@
+int main (int argc, char const *argv[])
+{
+return 0;
+}
Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,54 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_reading_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+
+@add_test_categories(['pyapi'])
+def test_reading_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(len(env.GetEntries()), 0)
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# Now there's a target, so at least PATH should exist
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.GetEntry("FOO"), None)
+self.assertEqual(env.GetEntry("BAR"), None)
+
+env.PutEntry("FOO=bar")
+env.PutEntry("BAR=foo")
+
+self.assertEqual(env.GetEntry("FOO"), "bar")
+self.assertEqual(env.GetEntry("BAR"), "foo")
+
+entries = env.GetEntries()
+self.assertEqual(len(entries), 2)
+
+for entry in env.GetEntries():
+self.assertTrue(entry in ["FOO=bar", "BAR=foo"])
Index: lldb/test/API/python_api/sbenvironment/Makefile
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -2643,6 +2655,7 @@
   LLDB_REGISTER_METHOD(lldb::SBInstructionList, SBTarget,
GetInstructionsWithFlavor,
(lldb::addr_t, const char *, const void *, size_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBTarget, GetEnvironment, ());
 }
 
 }
Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 250651.
wallace added a comment.

format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/Makefile
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
  lldb/test/API/python_api/sbenvironment/main.cpp

Index: lldb/test/API/python_api/sbenvironment/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/main.cpp
@@ -0,0 +1,4 @@
+int main (int argc, char const *argv[])
+{
+return 0;
+}
Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,54 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_reading_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+
+@add_test_categories(['pyapi'])
+def test_reading_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(len(env.GetEntries()), 0)
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# Now there's a target, so at least PATH should exist
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.GetEntry("FOO"), None)
+self.assertEqual(env.GetEntry("BAR"), None)
+
+env.PutEntry("FOO=bar")
+env.PutEntry("BAR=foo")
+
+self.assertEqual(env.GetEntry("FOO"), "bar")
+self.assertEqual(env.GetEntry("BAR"), "foo")
+
+entries = env.GetEntries()
+self.assertEqual(len(entries), 2)
+
+for entry in env.GetEntries():
+self.assertTrue(entry in ["FOO=bar", "BAR=foo"])
Index: lldb/test/API/python_api/sbenvironment/Makefile
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -2643,6 +2655,7 @@
   LLDB_REGISTER_METHOD(lldb::SBInstructionList, SBTarget,
GetInstructionsWithFlavor,
(lldb::addr_t, const char *, const void *, size_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBTarget, GetEnvironment, ());
 }
 
 }
Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 #include 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 250650.
wallace added a comment.

.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp
  lldb/source/API/SBTarget.cpp
  lldb/test/API/python_api/sbenvironment/Makefile
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
  lldb/test/API/python_api/sbenvironment/main.cpp

Index: lldb/test/API/python_api/sbenvironment/main.cpp
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/main.cpp
@@ -0,0 +1,4 @@
+int main (int argc, char const *argv[])
+{
+return 0;
+}
Index: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
@@ -0,0 +1,54 @@
+"""Test the SBEnvironment APIs."""
+
+
+
+from math import fabs
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBEnvironmentAPICase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+
+@add_test_categories(['pyapi'])
+def test_reading_platform_environment(self):
+env = self.dbg.GetSelectedPlatform().GetEnvironment()
+# We assume at least PATH is set
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+
+@add_test_categories(['pyapi'])
+def test_reading_target_environment(self):
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# There is no target, so env should be empty
+self.assertEqual(len(env.GetEntries()), 0)
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+env = self.dbg.GetSelectedTarget().GetEnvironment()
+# Now there's a target, so at least PATH should exist
+self.assertTrue(env.GetEntry("PATH") is not None)
+
+@add_test_categories(['pyapi'])
+def test_creating_and_modifying_environment(self):
+env = lldb.SBEnvironment()
+
+self.assertEqual(env.GetEntry("FOO"), None)
+self.assertEqual(env.GetEntry("BAR"), None)
+
+env.PutEntry("FOO=bar")
+env.PutEntry("BAR=foo")
+
+self.assertEqual(env.GetEntry("FOO"), "bar")
+self.assertEqual(env.GetEntry("BAR"), "foo")
+
+entries = env.GetEntries()
+self.assertEqual(len(entries), 2)
+
+for entry in env.GetEntries():
+self.assertTrue(entry in ["FOO=bar", "BAR=foo"])
Index: lldb/test/API/python_api/sbenvironment/Makefile
===
--- /dev/null
+++ lldb/test/API/python_api/sbenvironment/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -13,6 +13,7 @@
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
@@ -2388,6 +2389,17 @@
 m_opaque_sp->SetProcessLaunchInfo(launch_info.ref());
 }
 
+SBEnvironment SBTarget::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBTarget, GetEnvironment);
+  TargetSP target_sp(GetSP());
+
+  if (target_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -2643,6 +2655,7 @@
   LLDB_REGISTER_METHOD(lldb::SBInstructionList, SBTarget,
GetInstructionsWithFlavor,
(lldb::addr_t, const char *, const void *, size_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBTarget, GetEnvironment, ());
 }
 
 }
Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 #include 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/bindings/interface/SBPlatform.i:197-198
 
+lldb::SBEnvironment
+GetEnvironment();
+

What does it mean to get the environment from a platform? Fetching it from the 
remote platform as what ever binary was used to provide the connection?



Comment at: lldb/include/lldb/API/SBEnvironment.h:26
+
+  int Size();
+

labath wrote:
> s/int/size_t
return size_t or uint32_t.

Possibly rename to GetNumEntries() to be consistent with all of the other APIs 
that have a size and item accessor.



Comment at: lldb/include/lldb/API/SBEnvironment.h:28
+
+  const char *GetEntryAtIndex(int idx);
+

use size_t or uint32_t (which ever one you pick from Size() above



Comment at: lldb/include/lldb/API/SBPlatform.h:157
 
+  SBEnvironment GetEnvironment();
+

I know there isn't much header documentation here already, but it is a good 
time to start with any new functions. We would need to detail what this 
environment would be. Something like: "Return the environment variables 
contained in the remote platform connection process."



Comment at: lldb/source/API/SBEnvironment.cpp:46
+
+int SBEnvironment::Size() {
+  LLDB_RECORD_METHOD_NO_ARGS(int, SBEnvironment, Size);

```
size_t SBEnvironment::GetNumEnrtries()
```



Comment at: lldb/source/API/SBEnvironment.cpp:53
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int), idx);
+  return LLDB_RECORD_RESULT(m_opaque_up->getEnvp().get()[idx]);
+}

labath wrote:
> This will return a dangling pointer as Envp is a temporary object. It's 
> intended to be used to pass the environment object to execve-like function. 
> The current "state of the art" is to ConstStringify all temporary strings 
> that go out of the SB api. (Though I think we should start using std::string).
> 
> Also, constructing the Envp object just to fetch a single string out of it is 
> pretty expensive. You can fetch the desired result from the Environment 
> object directly.
> 
> Putting it all together, this kind of an API would be implemented via 
> something like:
> ```
>   if (idx >= m_opaque_up->size())
> return nullptr;
>   return ConstString(Environment::compose(*std::next(m_opaque_up.begin(), 
> idx)).AsCString();
> ```
> 
> ... but I am not sure this is really the right api. More on that in a 
> separate comment.
Yes, any C strings returned from the SB API should use 
ConstString(cstr).AsCString() as the ConstString puts the string into a string 
pool and there is no need to clients to destruct the string or take ownership.

No STL in the SB API please, so no std::string. std::string isn't stable on all 
platforms and competing C++ runtimes on different platforms can cause problems. 
If we need string ownership, we can introduce SBString as a class.

Please do bounds check this like Pavel requested as the index isn't guaranteed 
to be valid. If you bounds check first then your code above could be modified 
to return a ConstString().AsCString().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Thanks a lot for the feedback! I was thinking about adding features to this 
class as needed, but definitely I should make this API be more like a map


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Yes, a key/value approach would be a better API.  Looks like the Environment 
class would make that pretty easy to wrap up, as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure that this is the right API to represent an environment. The 
environment is more like a dictionary/map than an array. (The internal 
Environment object *is* a map, though this does not immediately mean the SB one 
should be too).

Even for your most immediate use case, you'll want to use the map-like 
properties of the environment (to "merge" the inherited environment and the 
user-provided values). With an api like this you'd have to implement all of 
that by yourself.

So, I am wondering if we shouldn't provide a more map-like interface here too. 
Rough proposal:

  const char *GetEntry(const char *name); // nullptr if not present
  void PutEntry(const char *string); // modeled on putenv(3)
  void SetEntry(const char *name, const char *value, bool overwrite); //modeled 
on setenv(3), maybe we can skip it if it's not needed now
  SBStringList GetEntries(); // if someone wants to enumerate all of them, 
maybe also skippable

If we don't want a map-like interface, then maybe we don't actually need a 
separate class, and an SBStringList would work just fine?

Maybe you could also refactor the other patch to use this new functionality 
(whatever it ends up being), so that we can see whether it makes sense at least 
for that use case..




Comment at: lldb/include/lldb/API/SBEnvironment.h:26-28
+  int Size();
+
+  const char *GetEntryAtIndex(int idx);

s/int/size_t



Comment at: lldb/source/API/SBEnvironment.cpp:1-2
+//===-- SBEnvironment.cpp
+//---===//
+//

fix formatting



Comment at: lldb/source/API/SBEnvironment.cpp:30-31
+SBEnvironment::SBEnvironment(const Environment )
+: m_opaque_up(new Environment()) {
+  *m_opaque_up = rhs;
+}

new(Environment(rhs))



Comment at: lldb/source/API/SBEnvironment.cpp:53
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int), idx);
+  return LLDB_RECORD_RESULT(m_opaque_up->getEnvp().get()[idx]);
+}

This will return a dangling pointer as Envp is a temporary object. It's 
intended to be used to pass the environment object to execve-like function. 
The current "state of the art" is to ConstStringify all temporary strings that 
go out of the SB api. (Though I think we should start using std::string).

Also, constructing the Envp object just to fetch a single string out of it is 
pretty expensive. You can fetch the desired result from the Environment object 
directly.

Putting it all together, this kind of an API would be implemented via something 
like:
```
  if (idx >= m_opaque_up->size())
return nullptr;
  return ConstString(Environment::compose(*std::next(m_opaque_up.begin(), 
idx)).AsCString();
```

... but I am not sure this is really the right api. More on that in a separate 
comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

Thanks for doing this, it will be useful!

I'll let Jonas comment on whether the Reproducer bindings are right, though it 
looks fine to me.

One of the very common uses of an SBEnvironment is the one in an SBTarget, 
which gets used to create new processes.  With what you have you can get access 
to it, so that's all good.  But you do it by getting the target's platform, and 
then the Environment from the platform.  It might be more discoverable to have 
the SBTarget hand it out directly as well, however.  And there is an 
lldb_private::Target::GetEnvironment, so it's pretty natural.

Also, you need some tests.  You can probably do a lot of it with unit tests.  
Then maybe a small API test that sets environment variables and fetches them 
out this way and checks that they are right.

The added API's are up to you.  The requested changes are for tests...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 250108.
wallace added a comment.

format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp

Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 #include "lldb/API/SBUnixSignals.h"
 #include "lldb/Host/File.h"
 #include "lldb/Target/Platform.h"
@@ -649,6 +651,17 @@
   return LLDB_RECORD_RESULT(SBUnixSignals());
 }
 
+SBEnvironment SBPlatform::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBPlatform, GetEnvironment);
+  PlatformSP platform_sp(GetSP());
+
+  if (platform_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(platform_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -740,6 +753,7 @@
(const char *));
   LLDB_REGISTER_METHOD(lldb::SBError, SBPlatform, SetFilePermissions,
(const char *, uint32_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBPlatform, GetEnvironment, ());
   LLDB_REGISTER_METHOD_CONST(lldb::SBUnixSignals, SBPlatform, GetUnixSignals,
  ());
 }
Index: lldb/source/API/SBEnvironment.cpp
===
--- /dev/null
+++ lldb/source/API/SBEnvironment.cpp
@@ -0,0 +1,69 @@
+//===-- SBEnvironment.cpp
+//---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/API/SBEnvironment.h"
+#include "SBReproducerPrivate.h"
+#include "Utils.h"
+#include "lldb/Utility/Environment.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBEnvironment::SBEnvironment() : m_opaque_up(new Environment()) {
+  LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBEnvironment);
+}
+
+SBEnvironment::SBEnvironment(const SBEnvironment )
+: m_opaque_up(new Environment()) {
+  LLDB_RECORD_CONSTRUCTOR(SBEnvironment, (const lldb::SBEnvironment &), rhs);
+
+  m_opaque_up = clone(rhs.m_opaque_up);
+}
+
+SBEnvironment::SBEnvironment(const Environment )
+: m_opaque_up(new Environment()) {
+  *m_opaque_up = rhs;
+}
+
+SBEnvironment::~SBEnvironment() = default;
+
+const SBEnvironment ::operator=(const SBEnvironment ) {
+  LLDB_RECORD_METHOD(const lldb::SBEnvironment &,
+ SBEnvironment, operator=,(const lldb::SBEnvironment &),
+ rhs);
+
+  if (this != )
+m_opaque_up = clone(rhs.m_opaque_up);
+  return LLDB_RECORD_RESULT(*this);
+}
+
+int SBEnvironment::Size() {
+  LLDB_RECORD_METHOD_NO_ARGS(int, SBEnvironment, Size);
+  return LLDB_RECORD_RESULT(m_opaque_up->size());
+}
+
+const char *SBEnvironment::GetEntryAtIndex(int idx) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int), idx);
+  return LLDB_RECORD_RESULT(m_opaque_up->getEnvp().get()[idx]);
+}
+
+namespace lldb_private {
+namespace repro {
+
+template <> void RegisterMethods(Registry ) {
+  LLDB_REGISTER_CONSTRUCTOR(SBEnvironment, ());
+  LLDB_REGISTER_CONSTRUCTOR(SBEnvironment, (const lldb::SBEnvironment &));
+  LLDB_REGISTER_METHOD(const lldb::SBEnvironment &,
+   SBEnvironment, operator=,(const lldb::SBEnvironment &));
+  LLDB_REGISTER_METHOD(int, SBEnvironment, Size, ());
+  LLDB_REGISTER_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int));
+}
+
+} // namespace repro
+} // namespace lldb_private
Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -35,6 +35,7 @@
   SBData.cpp
   SBDebugger.cpp
   SBDeclaration.cpp
+  SBEnvironment.cpp
   SBError.cpp
   SBEvent.cpp
   SBExecutionContext.cpp
Index: lldb/include/lldb/lldb-forward.h
===
--- lldb/include/lldb/lldb-forward.h
+++ lldb/include/lldb/lldb-forward.h
@@ -76,6 +76,7 @@
 

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: labath, clayborg.
Herald added subscribers: lldb-commits, mgorny.
Herald added a project: LLDB.

Inspired by https://reviews.llvm.org/D74636, I'm introducing a basic version of 
Environment in the API. More functionalities can be added as needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76111

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBEnvironment.i
  lldb/bindings/interface/SBPlatform.i
  lldb/bindings/interfaces.swig
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBEnvironment.h
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBEnvironment.cpp
  lldb/source/API/SBPlatform.cpp

Index: lldb/source/API/SBPlatform.cpp
===
--- lldb/source/API/SBPlatform.cpp
+++ lldb/source/API/SBPlatform.cpp
@@ -8,9 +8,11 @@
 
 #include "lldb/API/SBPlatform.h"
 #include "SBReproducerPrivate.h"
+#include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBLaunchInfo.h"
+#include "lldb/API/SBPlatform.h"
 #include "lldb/API/SBUnixSignals.h"
 #include "lldb/Host/File.h"
 #include "lldb/Target/Platform.h"
@@ -649,6 +651,17 @@
   return LLDB_RECORD_RESULT(SBUnixSignals());
 }
 
+SBEnvironment SBPlatform::GetEnvironment() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBEnvironment, SBPlatform, GetEnvironment);
+  PlatformSP platform_sp(GetSP());
+
+  if (platform_sp) {
+return LLDB_RECORD_RESULT(SBEnvironment(platform_sp->GetEnvironment()));
+  }
+
+  return LLDB_RECORD_RESULT(SBEnvironment());
+}
+
 namespace lldb_private {
 namespace repro {
 
@@ -740,6 +753,7 @@
(const char *));
   LLDB_REGISTER_METHOD(lldb::SBError, SBPlatform, SetFilePermissions,
(const char *, uint32_t));
+  LLDB_REGISTER_METHOD(lldb::SBEnvironment, SBPlatform, GetEnvironment, ()); 
   LLDB_REGISTER_METHOD_CONST(lldb::SBUnixSignals, SBPlatform, GetUnixSignals,
  ());
 }
Index: lldb/source/API/SBEnvironment.cpp
===
--- /dev/null
+++ lldb/source/API/SBEnvironment.cpp
@@ -0,0 +1,66 @@
+//===-- SBEnvironment.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/API/SBEnvironment.h"
+#include "SBReproducerPrivate.h"
+#include "Utils.h"
+#include "lldb/Utility/Environment.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBEnvironment::SBEnvironment() : m_opaque_up(new Environment()) {
+  LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBEnvironment);
+}
+
+SBEnvironment::SBEnvironment(const SBEnvironment ) : m_opaque_up(new Environment()) {
+  LLDB_RECORD_CONSTRUCTOR(SBEnvironment, (const lldb::SBEnvironment &), rhs);
+
+  m_opaque_up = clone(rhs.m_opaque_up);
+}
+
+SBEnvironment::SBEnvironment(const Environment ) : m_opaque_up(new Environment()) {
+  *m_opaque_up = rhs; 
+}
+
+SBEnvironment::~SBEnvironment() = default;
+
+const SBEnvironment ::operator=(const SBEnvironment ) {
+  LLDB_RECORD_METHOD(const lldb::SBEnvironment &,
+ SBEnvironment, operator=,(const lldb::SBEnvironment &), rhs);
+
+  if (this != )
+m_opaque_up = clone(rhs.m_opaque_up);
+  return LLDB_RECORD_RESULT(*this);
+}
+
+int SBEnvironment::Size() {
+  LLDB_RECORD_METHOD_NO_ARGS(int, SBEnvironment, Size);
+  return LLDB_RECORD_RESULT(m_opaque_up->size());
+}
+
+const char *SBEnvironment::GetEntryAtIndex(int idx) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int), idx);
+  return LLDB_RECORD_RESULT(m_opaque_up->getEnvp().get()[idx]);
+}
+
+namespace lldb_private {
+namespace repro {
+
+template <>
+void RegisterMethods(Registry ) {
+  LLDB_REGISTER_CONSTRUCTOR(SBEnvironment, ());
+  LLDB_REGISTER_CONSTRUCTOR(SBEnvironment, (const lldb::SBEnvironment &));
+  LLDB_REGISTER_METHOD(const lldb::SBEnvironment &,
+   SBEnvironment, operator=,(const lldb::SBEnvironment &));
+  LLDB_REGISTER_METHOD(int, SBEnvironment, Size, ()); 
+  LLDB_REGISTER_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int));
+}
+
+}
+}
Index: lldb/source/API/CMakeLists.txt
===
--- lldb/source/API/CMakeLists.txt
+++ lldb/source/API/CMakeLists.txt
@@ -35,6 +35,7 @@
   SBData.cpp
   SBDebugger.cpp
   SBDeclaration.cpp
+  SBEnvironment.cpp
   SBError.cpp
   SBEvent.cpp
   SBExecutionContext.cpp
Index: lldb/include/lldb/lldb-forward.h