[llvm] r260488 - [readobj] Handle ELF files with no section table or with no program headers.

[Michael Spencer via llvm-commits]

On Tue, Feb 16, 2016 at 6:12 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> On 12 February 2016 at 19:29, Michael Spencer <bigcheesegs at gmail.com> wrote:
>> On Fri, Feb 12, 2016 at 2:15 PM, Rafael Espíndola
>> <rafael.espindola at gmail.com> wrote:
>>> There are a few problems with this patch
>>>
>>>>    uint32_t Index = Sym->st_shndx;
>>>>    if (Index == ELF::SHN_XINDEX)
>>>> -    return getSection(getExtendedSymbolTableIndex(Sym, SymTab, ShndxTable));
>>>> +    return getSection(
>>>> +        getExtendedSymbolTableIndex(Sym, symbol_begin(SymTab), ShndxTable));
>>>
>>>
>>> Why call the more complicated version in here?
>>
>> I was going to remove the const Elf_Shdr * version as this was the
>> only remaining use, but lld uses it so I kept the forwarder. I forgot
>> to switch this back to using it.
>>
>>>
>>>>
>>>> -RUN: not llvm-readobj -dynamic-table \
>>>> -RUN:   %p/Inputs/corrupt-invalid-virtual-addr.elf.x86-64 2>&1 | \
>>>> -RUN:   FileCheck --check-prefix=VIRTADDR %s
>>>> -
>>>> -VIRTADDR: Virtual address is not in any segment
>>>
>>> Why is this not an error anymore?
>>
>> Because it was checking the validity of addresses that were never
>> accessed. There's no reason to bail out when the user hasn't requested
>> us to print invalid data. Specifically the code that emitted this
>> error was blocking dynamic-table-exec-no-phdrs.x86 from dumping data
>> even though we had full section table information.
>
> The data is still invalid and it should still be reported when found
> to be broken. If it was not needed on this test, you should should
> have added a new test.

It may be fine to emit a warning, but there's no need to abort unless
the user wanted to do something that can't (or is unreasonable to) be
done. Other tests caught actual uses of invalid data.

>
>
>>>
>>> Could you revert this and send it back for review? Please split it up too.
>>
>> Unless there is a glaring design issue with the approach taken in this
>> patch I don't believe that it is reasonable to revert. I'm glad to fix
>> any issues you've found with the code.
>
>
> There is. You deleted error checking code without review.
>
> Cheers,
> Rafael

That was not an acceptable reason to revert this patch, and there is
zero precedent for doing this. You just decided that you were right
and reverted a patch without discussion, and went against the llvm
developer policy. Do not do this in the future.

- Michael Spencer