[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 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