[Lldb-commits] [PATCH] D127999: [lldb] fix stepping through POSIX trampolines

[Michael Daniels via Phabricator via lldb-commits]

mdaniels added inline comments.


================
Comment at: lldb/test/API/lang/cpp/step-into-namespace/TestStepIntoNamespace.py:5
+
+class StepIntoNamespace(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
----------------
labath wrote:
> Am I correct in assuming that the "namespace" part here comes from the fact that global functions (e.g. `_Z3foov` aka `foo()`) would be found according to their base name (`foo`) or something?
> 
> If yes, that's fine, but it would still be nice to mention (say in a comment or something) that the shared library is an important aspect of the test ("TestStepIntoNamespace" does not convey that notion).
> 
> If this can also be reproduced with regular functions, then it'd be better to rename the test into something else (which includes the word "trampoline").
The lookup will be done with the full demangled name, so in this case it would be "foo::foo()".

The namespace part was just because I had trouble reproducing when reducing the testcase to a plain function call. Though I tried it again and can reproduce the issue fine, so was likely user error on my part.

I will rename as suggested, thanks.


================
Comment at: lldb/test/API/lang/cpp/step-into-namespace/TestStepIntoNamespace.py:8
+
+    def test(self):
+        self.build()
----------------
labath wrote:
> mdaniels wrote:
> > clayborg wrote:
> > > Do we want to limit this to linux? I am not sure this will pass the windows buildbots if we don't restrict it
> > Looking at other tests that load a shared library, it seems I should have at least
> > 
> > ```
> > @skipIfRemote
> > @skipIfWindows
> > ```
> > But I can also just restrict it to the platforms I am able to test on locally, linux and darwin, if there is still concern that other platforms might fail.
> Neither of these is strictly necessary. Remote shared library tests need additional care as one has to copy the shared library to the remote system, but afaik, you're using the correct incantation which should do that automatically.
> 
> Windows shared library support is not at the level of other platforms, but a simple setup like this should work. Or rather: if it fails, it will probably be due to the trampoline aspect, and not the shared library loading. So I think you don't need to add any decorators right now. We can add xfail-windows if it turns out to be failing.
Good to know, thanks for the feedback


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

https://reviews.llvm.org/D127999