r221211

[Kevin Enderby]

Hi Aaron,

Thanks for the email.  I’ll get this fixed up.  Comments below.

Kev

On Nov 11, 2014, at 6:23 AM, Aaron Ballman <aaron at aaronballman.com> wrote:

> It seems the mail server ate the commit notification about this
> change,

Oddly I see this too with some but not all of my commits.  No idea how to track this down.  I do all the commits from the same machine and the same set of sources.

> so I am pasting in just the relevant parts instead of
> responding to a commit email.
> 
> --- E:/temp/MachODump.cpp-rev221210.svn000.tmp.cpp Wed Oct 29 17:28:24 2014
> +++ E:/temp/MachODump.cpp-rev221211.svn000.tmp.cpp Mon Nov  3 19:43:16 2014
> @@ -248,6 +248,21 @@ struct DisassembleInfo {
>   BindTable *bindtable;
> };
> 
> +// GuessSymbolName is passed the address of what might be a symbol and a
> +// pointer to the DisassembleInfo struct.  It returns the name of a symbol
> +// with that address or nullptr if no symbol is found with that address.
> +static const char *GuessSymbolName(uint64_t value,
> +                                   struct DisassembleInfo *info) {
> +  const char *SymbolName = nullptr;
> +  // A DenseMap can't lookup up some values.
> +  if (value != 0xffffffffffffffffULL && value != 0xfffffffffffffffeULL) {
> +    StringRef name = info->AddrMap->lookup(value);
> +    if (!name.empty())
> +      SymbolName = name.data();
> +  }
> +  return SymbolName;
> +}
> +
> // SymbolizerGetOpInfo() is the operand information call back function.
> // This is called to get the symbolic information for operand(s) of an
> // instruction when it is being done.  This routine does this from
> @@ -281,6 +296,83 @@ int SymbolizerGetOpInfo(void *DisInfo, uint64_t Pc
> 
>   unsigned int Arch = info->O->getArch();
>   if (Arch == Triple::x86) {
> +    if (Size != 1 && Size != 2 && Size != 4 && Size != 0)
> +      return 0;
> +    // First search the section's relocation entries (if any) for an entry
> +    // for this section offset.
> +    uint32_t sect_addr = info->S.getAddress();
> +    uint32_t sect_offset = (Pc + Offset) - sect_addr;
> +    bool reloc_found = false;
> +    DataRefImpl Rel;
> +    MachO::any_relocation_info RE;
> +    bool isExtern = false;
> +    SymbolRef Symbol;
> +    bool r_scattered = false;
> +    uint32_t r_value, pair_r_value, r_type;
> +    for (const RelocationRef &Reloc : info->S.relocations()) {
> +      uint64_t RelocOffset;
> +      Reloc.getOffset(RelocOffset);
> +      if (RelocOffset == sect_offset) {
> +        Rel = Reloc.getRawDataRefImpl();
> +        RE = info->O->getRelocation(Rel);
> +        r_scattered = info->O->isRelocationScattered(RE);
> +        if (r_scattered) {
> +          r_value = info->O->getScatteredRelocationValue(RE);
> +          r_type = info->O->getScatteredRelocationType(RE);
> +          if (r_type == MachO::GENERIC_RELOC_SECTDIFF ||
> +              r_type == MachO::GENERIC_RELOC_LOCAL_SECTDIFF) {
> +            DataRefImpl RelNext = Rel;
> +            info->O->moveRelocationNext(RelNext);
> +            MachO::any_relocation_info RENext;
> +            RENext = info->O->getRelocation(RelNext);
> +            if (info->O->isRelocationScattered(RENext))
> +              pair_r_value = info->O->getPlainRelocationSymbolNum(RENext);
> +            else
> +              return 0;
> +          }
> +        } else {
> +          isExtern = info->O->getPlainRelocationExternal(RE);
> +          if (isExtern) {
> +            symbol_iterator RelocSym = Reloc.getSymbol();
> +            Symbol = *RelocSym;
> +          }
> +        }
> +        reloc_found = true;
> +        break;
> +      }
> +    }
> +    if (reloc_found && isExtern) {
> +      StringRef SymName;
> +      Symbol.getName(SymName);
> +      const char *name = SymName.data();
> +      op_info->AddSymbol.Present = 1;
> +      op_info->AddSymbol.Name = name;
> +      // For i386 extern relocation entries the value in the instruction is
> +      // the offset from the symbol, and value is already set in
> op_info->Value.
> +      return 1;
> +    }
> +    if (reloc_found && (r_type == MachO::GENERIC_RELOC_SECTDIFF ||
> +                        r_type == MachO::GENERIC_RELOC_LOCAL_SECTDIFF)) {
> 
> This use of r_type may be using an uninitialized variable, and is causing:
> 
> MachODump.cpp: In function ‘int SymbolizerGetOpInfo(void*, uint64_t,
> uint64_t, uint64_t, int, void*)’:
> MachODump.cpp:385:5: warning: ‘r_type’ may be used uninitialized in
> this function [-Wuninitialized]
> 
> (r_type is only be initialized if r_scattered is true.)
> 
> I wasn't quite certain what the correct fix for this would be.

Yep I can see that logic.  The fix I think is pretty simple and would be this:

@@ -346,10 +347,10 @@
       if (RelocOffset == sect_offset) {
         Rel = Reloc.getRawDataRefImpl();
         RE = info->O->getRelocation(Rel);
+        r_type = info->O->getAnyRelocationType(RE);
         r_scattered = info->O->isRelocationScattered(RE);
         if (r_scattered) {
           r_value = info->O->getScatteredRelocationValue(RE);
-          r_type = info->O->getScatteredRelocationType(RE);
           if (r_type == MachO::GENERIC_RELOC_SECTDIFF ||
               r_type == MachO::GENERIC_RELOC_LOCAL_SECTDIFF) {
             DataRefImpl RelNext = Rel;

I’ve got some other changes I’m working on in that file so let me clean that out and test with just the above change and commit that first.  Hopefully we’ll all see a commit message this time :)

> 
> +      const char *add = GuessSymbolName(r_value, info);
> +      const char *sub = GuessSymbolName(pair_r_value, info);
> +      uint32_t offset = value - (r_value - pair_r_value);
> +      op_info->AddSymbol.Present = 1;
> +      if (add != nullptr)
> +        op_info->AddSymbol.Name = add;
> +      else
> +        op_info->AddSymbol.Value = r_value;
> +      op_info->SubtractSymbol.Present = 1;
> +      if (sub != nullptr)
> +        op_info->SubtractSymbol.Name = sub;
> +      else
> +        op_info->SubtractSymbol.Value = pair_r_value;
> +      op_info->Value = offset;
> +      return 1;
> +    }
> +    // TODO:
> +    // Second search the external relocation entries of a fully linked image
> +    // (if any) for an entry that matches this segment offset.
> +    // uint32_t seg_offset = (Pc + Offset);
> 
> Thanks!
> 
> ~Aaron