clayborg 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 =
----------------
labath wrote:
> 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.
I checked ObjectFileMachO and it indeed sets external and weak separately. For
the debugger, we just want to know if the symbol is externally visible from an
object file standpoint. So we can fix ObjectFileELF.cpp with code:
```
const bool is_weak = symbol.getBinding() == STB_WEAK;
Symbol dc_symbol(
i + start_id, // ID is the original symbol table index.
mangled,
symbol_type, // Type of this symbol
is_global | is_weak, // Is this globally visible?
false, // Is this symbol debug info?
false, // Is this symbol a trampoline?
false, // Is this symbol artificial?
AddressRange(symbol_section_sp, // Section in which this symbol is
// defined or null.
symbol_value, // Offset in section or symbol value.
symbol.st_size), // Size in bytes of this symbol.
symbol_size_valid, // Symbol size is valid
has_suffix, // Contains linker annotations?
flags); // Symbol flags.
if (is_weak)
dc_symbol.SetIsWeak(true);
```
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
