[lld] r219176 - Do not use llvm_unreachable at reachable code.

[Saleem Abdulrasool]

On Tue, Oct 7, 2014 at 12:42 PM, Rui Ueyama <ruiu at google.com> wrote:

> On Mon, Oct 6, 2014 at 7:55 PM, Saleem Abdulrasool <compnerd at compnerd.org>
> wrote:
>
>> On Mon, Oct 6, 2014 at 6:42 PM, Rui Ueyama <ruiu at google.com> wrote:
>>
>>> On Mon, Oct 6, 2014 at 6:25 PM, Saleem Abdulrasool <
>>> compnerd at compnerd.org> wrote:
>>>
>>>> On Mon, Oct 6, 2014 at 5:40 PM, Rui Ueyama <ruiu at google.com> wrote:
>>>>
>>>>> Author: ruiu
>>>>> Date: Mon Oct  6 19:40:54 2014
>>>>> New Revision: 219176
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=219176&view=rev
>>>>> Log:
>>>>> Do not use llvm_unreachable at reachable code.
>>>>>
>>>>> These lines can be reachable if we give a broken or unsupported
>>>>> input object file.
>>>>>
>>>>
>>>> Should we really be relying on this for invalid input detection?  We
>>>> should immediately identify if the object file is incorrect when we snarf
>>>> the input object and it mismatches the target (I recently added that
>>>> support).  Yes, I agree that it could be a broken input, but, shouldn't we
>>>> have better detection for that in libObject rather than just happily
>>>> consuming it?
>>>>
>>>
>>> This report_fatal_error is for unknown relocation types. That's
>>> different from file magic. I don't think we want libObject to check if a
>>> relocation type is within a known range for an architecture. If we make
>>> libObject to check for that, it would return a reference only when a
>>> reference type value is valid. That's fine. But we'd still want to keep
>>> this now-really-unreachable error check anyway to cover all switch cases.
>>> That doesn't seem a good deal to me to error out on an broken input file,
>>> which is a rare event.
>>>
>>
>> Yes, I get that the error reporting here is for a different scenario.  I
>> was responding to the commit message: if the object file is unsupported,
>> its either because the format is incorrect (how did we get here in this
>> case?  the registry shouldn't have dispatched to us) or because the file
>> magic indicates that the file is for a different target (already
>> diagnosed).  That leaves the case of a broken/corrupt object file, in which
>> case, libObject seems like the right place to validate the input.
>>
>> I completely agree that libObject should not be aware of the details of
>> the relocation types and their ranges.  I think that Nick's approach of the
>> error message + code for a bad relocation (or out of range) is better than
>> the report_fatal_error.
>>
>
> So you want libObject to validate the input, but at the same time you are
> thinking that libObject shouldn't be aware the details of relocation types
> and their ranges? It seems they are conflicting goal to me. What if an
> object file being read is almost correct, but only a byte at a relocation
> type is corrupted? In order to verify if a relocation number is valid,
> libObject needs to know the valid range, no?
>

Oh, we are talking about different things!  I assumed you meant branch
range (the distance a relocation type can address) rather than the range of
values representing a relocation.  Obviously, libObject knows that a series
of bytes indicates a relocation, and it knows the type of relocation it
maps to.


>
>>>>
>>>>> Modified:
>>>>>     lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
>>>>>
>>>>> Modified: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp?rev=219176&r1=219175&r2=219176&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp (original)
>>>>> +++ lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp Mon Oct  6
>>>>> 19:40:54 2014
>>>>> @@ -558,7 +558,7 @@ void AtomChunk::applyRelocationsX86_32(u
>>>>>              targetAddr - getSectionStartAddr(targetAddr, sectionRva);
>>>>>          break;
>>>>>        default:
>>>>> -        llvm_unreachable("Unsupported relocation kind");
>>>>> +        llvm::report_fatal_error("Unsupported relocation kind");
>>>>>        }
>>>>>      }
>>>>>    }
>>>>> @@ -618,7 +618,7 @@ void AtomChunk::applyRelocationsX86_64(u
>>>>>          break;
>>>>>        default:
>>>>>          llvm::errs() << "Kind: " << (int)ref->kindValue() << "\n";
>>>>> -        llvm_unreachable("Unsupported relocation kind");
>>>>> +        llvm::report_fatal_error("Unsupported relocation kind");
>>>>>        }
>>>>>      }
>>>>>    }
>>>>> @@ -934,7 +934,7 @@ StringRef chooseSectionByContent(const D
>>>>>    }
>>>>>    llvm::errs() << "Atom: contentType=" << atom->contentType()
>>>>>                 << " permission=" << atom->permissions() << "\n";
>>>>> -  llvm_unreachable("Failed to choose section based on content");
>>>>> +  llvm::report_fatal_error("Failed to choose section based on
>>>>> content");
>>>>>  }
>>>>>
>>>>>  typedef std::map<StringRef, std::vector<const DefinedAtom *> >
>>>>> AtomVectorMap;
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Saleem Abdulrasool
>>>> compnerd (at) compnerd (dot) org
>>>>
>>>
>>>
>>
>>
>> --
>> Saleem Abdulrasool
>> compnerd (at) compnerd (dot) org
>>
>
>


-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141007/64499803/attachment.html>