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

[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 probably don't need to check for IsWeak() as we wouldn't want an internal + weak symbol (if there is such a thing) to match and be used.



================
Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:36
 
 bool lldb_private::InferiorCallMmap(Process *process, addr_t &allocated_addr,
                                     addr_t addr, addr_t length, unsigned prot,
----------------
Might be nice to return a llvm::Error here instead of pool? Maybe to help explain what went wrong? like any of:

"thread required to call mmap"
"couldn't find any symbols named 'mmap'"
"no external symbols named 'mmap' were found"
"mmap call failed" (for when it returns UINT32_MAX for 4 byte addresses or UINT64_MAX for 8 byte addresses"

It have been nice to see a nice error message during the expression evaluation prior to this fix. At the very least we should log to the expression log channel that mmap failed if we choose not to return an error.


================
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 =
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868