[lld] r236726 - [ARM] llvm_unreachable => make_*_reloc_error in group relocs

[Rafael Espíndola]

If it is really missing functionality you should not be doing

 return make_dynamic_error_code(
+      "Negative offsets for group relocations are not implemented");

Just use a report_fatal_error.

On the llvm side lib/Object suffered (and still does) a lot from a ton
of error handling that was added "just in case". One should not add
proper error handling until you can add a .o file that shows lld
handling the error.



On 22 May 2015 at 04:50, Denis Protivensky
<dprotivensky at accesssoftek.com> wrote:
> Once again: tests covering *missing* functionality are useless.
> It's the same as adding tests to missing relocations and catching error
> output informing that relocations are missing.
>
> When and if someone adds logic to handle negative offsets for group
> relocations, they would add proper test cases.
> For now on the linker will inform users that it can't perform handling of
> group relocations with negative offsets, which is perfectly enough for
> diagnostics.
>
> Thanks,
>   Denis.
>
>
> On 05/22/2015 10:10 AM, Filipe Cabecinhas wrote:
>
> It's adding tests for actual functionality:
>   If the offset is negative, error out with "blah".
>
> Regards,
>
>   Filipe
>
> On Thursday, May 21, 2015, Denis Protivensky <dprotivensky at accesssoftek.com>
> wrote:
>>
>> Yes, I know that. It's not error handling, but rather an indication of
>> missing functionality.
>> I consider adding tests for not implemented functionality to be excessive.
>>
>> On 05/22/2015 02:52 AM, Rafael Espíndola wrote:
>>
>> It is good practice to include a test case when adding error handling.
>>
>> On May 7, 2015 9:48 AM, "Denis Protivensky"
>> <dprotivensky at accesssoftek.com> wrote:
>>>
>>> Author: denis-protivensky
>>> Date: Thu May  7 08:41:44 2015
>>> New Revision: 236726
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=236726&view=rev
>>> Log:
>>> [ARM] llvm_unreachable => make_*_reloc_error in group relocs
>>>
>>> Modified:
>>>     lld/trunk/lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
>>>
>>> Modified: lld/trunk/lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp?rev=236726&r1=236725&r2=236726&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
>>> (original)
>>> +++ lld/trunk/lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp Thu May
>>> 7 08:41:44 2015
>>> @@ -112,6 +112,11 @@ static Reference::Addend readAddend(cons
>>>    }
>>>  }
>>>
>>> +static inline std::error_code make_unsupported_range_group_reloc_error()
>>> {
>>> +  return make_dynamic_error_code(
>>> +      "Negative offsets for group relocations are not implemented");
>>> +}
>>> +
>>>  static inline std::error_code applyArmReloc(uint8_t *location, uint32_t
>>> result,
>>>                                              uint32_t mask = 0xFFFFFFFF)
>>> {
>>>    assert(!(result & ~mask));
>>> @@ -475,10 +480,8 @@ static std::error_code relocR_ARM_ALU_PC
>>>  static std::error_code relocR_ARM_ALU_PC_G0_NC(uint8_t *location,
>>> uint64_t P,
>>>                                                 uint64_t S, int64_t A) {
>>>    int32_t result = (int32_t)(S + A - P);
>>> -
>>>    if (result < 0)
>>> -    llvm_unreachable(
>>> -        "Negative offsets for group relocations has not been
>>> implemented");
>>> +    return make_unsupported_range_group_reloc_error();
>>>
>>>    DEBUG(llvm::dbgs() << "\t\tHandle " << LLVM_FUNCTION_NAME << " -";
>>>          llvm::dbgs() << " S: 0x" << Twine::utohexstr(S);
>>> @@ -494,10 +497,8 @@ static std::error_code relocR_ARM_ALU_PC
>>>  static std::error_code relocR_ARM_ALU_PC_G1_NC(uint8_t *location,
>>> uint64_t P,
>>>                                                 uint64_t S, int64_t A) {
>>>    int32_t result = (int32_t)(S + A - P);
>>> -
>>>    if (result < 0)
>>> -    llvm_unreachable(
>>> -        "Negative offsets for group relocations has not been
>>> implemented");
>>> +    return make_unsupported_range_group_reloc_error();
>>>
>>>    DEBUG(llvm::dbgs() << "\t\tHandle " << LLVM_FUNCTION_NAME << " -";
>>>          llvm::dbgs() << " S: 0x" << Twine::utohexstr(S);
>>> @@ -513,10 +514,8 @@ static std::error_code relocR_ARM_ALU_PC
>>>  static std::error_code relocR_ARM_LDR_PC_G2(uint8_t *location, uint64_t
>>> P,
>>>                                              uint64_t S, int64_t A) {
>>>    int32_t result = (int32_t)(S + A - P);
>>> -
>>>    if (result < 0)
>>> -    llvm_unreachable(
>>> -        "Negative offsets for group relocations has not been
>>> implemented");
>>> +    return make_unsupported_range_group_reloc_error();
>>>
>>>    DEBUG(llvm::dbgs() << "\t\tHandle " << LLVM_FUNCTION_NAME << " -";
>>>          llvm::dbgs() << " S: 0x" << Twine::utohexstr(S);
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
>
> --
>   F
>
>