[lld] r211921 - [mach-o] refactor x86_64 relocation handling.

[Nick Kledzik]

On Jun 30, 2014, at 4:54 AM, Tim Northover <t.p.northover at gmail.com> wrote:

> Hi Nick,
> 
> Couple of things I noticed while looking through. Feel free to ignore:
> 
>>   // Debug logging of symbols.
>>   //for (const Symbol *sym : symbols)
>> -  //  llvm::errs() << "sym: " << sym->value << ", " << sym->name << "\n";
>> +  //  llvm::errs() << "  sym: "
>> +  //    << llvm::format("0x%08llx ", (uint64_t)sym->value)
>> +  //    << ", " << sym->name << "\n";
> 
> Should we put this in DEBUG or something, and make it official? I made
> use of it the other day too.
Feel free to do that. Making up the name that will trigger it is always the hardest 
part for me when adding DEBUG_WITH_TYPE ;-)


> 
>> +      case atomizeCFString:
>> +        // Break section up into compact unwind entries.
>> +        size = is64 ? 32 : 16;
>> +        break;
> 
> This looks like a comment copy/paste error.
Yes. I’ll fix.

> 
>> +    MachODefinedAtom *inAtom = file.findAtomCoveringAddress(section,
>> +                                                            reloc.offset,
>> +                                                            &offsetInAtom);
>> +    if (!inAtom)
>> +      return make_dynamic_error_code(Twine("no atom at r_address"));
> 
> Isn't this more of an assertion-failure error? It seems like a gappy
> section shouldn't even be able to exist in an ill-formed MachO file.
It means a malformed mach-o file.  For instance, the sect index in a
relocation is wrong.  I try to turn all malformed file issues into errors,
and leave asserts for programming errors in the linker itself (not
programming errors in the tool that generated the bad .o file).


> 
>> bool KindHandler_x86_64::isPointer(const Reference &ref) {
>> -  return (ref.kindValue() == X86_64_RELOC_UNSIGNED);
>> +  if (ref.kindNamespace() != Reference::KindNamespace::mach_o)
>> +    return false;
>> +  assert(ref.kindArch() == Reference::KindArch::x86_64);
>> +  return (ref.kindValue() == pointer64);
> 
> Isn't pointer64Anon also a pointer, at least for the purposes
> "isPointer" gets used for
Yes.  Good catch! I’ll fix.

-Nick