labath added inline comments.
================
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:
> labath wrote:
> > aadsm wrote:
> > > labath wrote:
> > > > 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.
> > > All (with the exception of the one overriden by the leak sanitizer) mmap
> > > symbols in the symbol table are Weak bound but none are External bound,
> > > this was the main reason that lead me to investigate the difference
> > > between the two.
> > >
> > > Not sure how to check how lldb interprets the Weak overall, but I think
> > > it kind of does, because that's what I saw when I dumped the symbol table:
> > > ```
> > > (lldb) target modules dump symtab libc.so.6
> > > Debug symbol
> > > |Synthetic symbol
> > > ||Externally Visible
> > > |||
> > > Index UserID DSX Type File Address/Value Load Address
> > > Size Flags Name
> > > ------- ------ --- --------------- ------------------ ------------------
> > > ------------------ ---------- ----------------------------------
> > > ...
> > > [ 6945] 6947 X Code 0x000000000010b5e0 0x00007ffff69db5e0
> > > 0x00000000000000f9 0x00000012 __mmap
> > > ...
> > > ```
> > >
> > > Another solution I thought was to add a IsLocal to the Symbol (and use
> > > !IsLocal) but then not really sure how to implement this for all Symbols
> > > not ELFSymbol.
> > Interesting. I guess I should have verified my assumptions. With that in
> > mind, I think checking for both weak and external (strong) symbols is fine.
> I think this is a bug in the ObjectFileELF where it is setting weak but not
> external for weak symbols. Weak symbols would be useless if they weren't
> external as the first one to get used should end up winning and all other
> weak symbols should resolve to the first symbol to come along that matches
> when dyld does a lookup. So I think we should verify this with some linker
> folks and remove IsWeak from this if statement, and fix the ObjectFileELF
> symbol table parsing code.
That sounds reasonable. The documentation for SBSymbol::IsExternal says "A read
only property that returns a boolean value that indicates if this symbol is
externally visible (exported) from the module that contains it." An elf weak
symbol definitely fits that definition.
Note that this would be a different meaning of "external" than what is used for
the "external" llvm [[ https://llvm.org/docs/LangRef.html#linkage-types |
linkage type ]]. There, it refers to a specific linkage type (externally
visible and strong), distinct from weak (and linkonce, etc) linkage types,
though most of them are still externally visible.
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
