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

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

Reply via email to