[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Regardless of the future of the syscall approach, I think limiting ourselves to external mmap symbols is a good idea. And if it fixes your problem -- great.

It would be good to have a test for that, though. Maybe something like this would do?

  static void *mmap() { return (void *)47; }
  
  int call_me() { return 42; }
  
  int main() {
    mmap();
    return call_me();
  }

right now, I am unable to evaluate `call_me()` with lldb. I guess your patch should make that possible?



================
Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:54
+    if (sc_list.GetContextAtIndex(i, sc) &&
+        (sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
       const uint32_t range_scope =
----------------
clayborg wrote:
> aadsm wrote:
> > clayborg wrote:
> > > Why are we checking "IsWeak()" here?
> > A weak symbol is also an external symbol, but it's weak in the sense that another external symbol with the same name will take precedence over it (as far as I understood).
> I think we only need to check for external here. Any weak symbol will also need to be external, but if it isn't we don't want that symbol.
Your understanding is correct, at least at the object file level -- I'm not sure whether `IsWeak()` implies `IsExternal()` inside lldb. However, I would actually argue for removal of IsWeak() for a different reason -- any weak definition of mmap is not going to be used by the process since libc already has a strong definition of that symbol.

If we really end up in a situation where we only have a weak mmap symbol around, then this is probably a sufficiently strange setup that we don't want to be randomly calling that function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868