[llvm] r258298 - [LTO] Fix error reporting when a file passed to libLTO is invalid or non-existent
Petr Pavlu via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 10 01:38:42 PST 2016
The following seems to happen:
lto_module_create_in_local_context()
-> LTOModule::createInLocalContext()
-> LTOModule::createInContext()
-> LTOModule::makeLTOModule()
-> parseBitcodeFileImpl()
createInLocalContext() passes nullptr as Context to createInContext() which
passes it unchanged to makeLTOModule(). This function sees the Context is
nullptr and creates a new temporary LLVMContext but this default context exits
on any error. parseBitcodeFileImpl() then reports an error through this context
and so the program exits.
There are other situations when an error can be reported through this context
(by functions that parseBitcodeFileImpl() calls), the patch only added one
missing. Another problem is that using a different context than LTOContext from
tools/lto/lto.cpp will cause that sLastErrorString does not get set on error
(i.e. even if lto_module_create_in_local_context() returned NULL without
exiting, lto_get_error_message() would not return any error message
afterwards).
One possible fix could be to create a temporary LLVMContext in
lto_module_create_in_local_context(), set its diagnostics handler in the same
way LTOContext is set up and then call createInContext() giving it this
context. I can look if such a solution is feasible in a few days. As explained
above, I think that reverting the patch would only hide the underlying problem.
Petr
On 08/02/16 16:02, Frédéric Riss wrote:
> Hi Petr,
>
>> On Feb 8, 2016, at 1:48 AM, Petr Pavlu <petr.pavlu at arm.com> wrote:
>>
>> I have troubles to see how this commit affects
>> `lto_module_is_object_file_in_memory()`. I can revert it but I would like to
>> first make sure this is a change that really causes the problem you are
>> seeing. Could you please send me a minimal example that shows the problem?
>
> yes, you are right. I misread the backtrace, sorry about that. The function that fails is not lto_module_is_object_file_in_memory(), but lto_module_create_in_local_context().
> It looks like it was able to return NULL before before, when it now simply exits to process.
>
> What is the API contract for these functions? Should one be able to call the latter on any file or is it fine to exit on a non-bitcode one?
>
> Weirdly enough, cctools tests the file with lto_module_is_object_file_in_memory() but for some reason the test returns true on a non-bitcode file. I’ll dig into this.
>
> Fred
>
>
>> My test case:
>> $ cat test.c
>> #include <stdio.h>
>> #include "llvm-c/lto.h"
>>
>> int main(void)
>> {
>> lto_bool_t is;
>>
>> is = lto_module_is_object_file_in_memory("\xde\xc0\x17\x0b", 4);
>> printf("bitcode -> %d.\n", is);
>>
>> is = lto_module_is_object_file_in_memory("abcd", 4);
>> printf("not -> %d.\n", is);
>>
>> return 0;
>> }
>> $ cc -Illvm/include -Lllvm/build/lib -lLTO test.c
>> $ LD_LIBRARY_PATH=$PWD/llvm/build/lib ./a.out
>> bitcode -> 1.
>> not -> 0.
>>
>> Thanks,
>> Petr
>>
>> On 06/02/16 06:32, Duncan P. N. Exon Smith wrote:
>>> Unfortunately, this commit changes the behaviour of:
>>> ```
>>> extern lto_bool_t
>>> lto_module_is_object_file_in_memory(const void* mem, size_t length);
>>> ```
>>>
>>> Because of this commit, `lto_module_is_object_file_in_memory()` is
>>> completely broken: instead of returning 0 when it's not bitcode, it
>>> now calls `exit()`.
>>>
>>> Can you revert this until we come up with another solution?
>>>
>>> (Sorry for the delay on this, I didn't realize the problem until we
>>> started getting weird crashes in cctools when trying out new builds
>>> of clang+libLTO.dylib. You can see the caller here, inside
>>> `is_llvm_bitcode_from_memory()`:
>>> http://www.opensource.apple.com/source/cctools/cctools-877.5/libstuff/lto.c
>>> )
>>>
>>>> On 2016-Jan-20, at 01:03, Petr Pavlu via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>>
>>>> Author: petr.pavlu
>>>> Date: Wed Jan 20 03:03:42 2016
>>>> New Revision: 258298
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=258298&view=rev
>>>> Log:
>>>> [LTO] Fix error reporting when a file passed to libLTO is invalid or non-existent
>>>>
>>>> This addresses PR26060 where function lto_module_create() could return nullptr
>>>> but lto_get_error_message() returned an empty string.
>>>>
>>>> The error() call after LTOModule::createFromFile() in llvm-lto is then removed
>>>> because any error from this function should go through the diagnostic handler in
>>>> llvm-lto which will exit the program. The error() call was added because this
>>>> previously did not happen when the file was non-existent. This is fixed by the
>>>> patch. (The situation that llvm-lto reports an error when the input file does
>>>> not exist is tested by llvm/tools/llvm-lto/error.ll).
>>>>
>>>> Differential Revision: http://reviews.llvm.org/D16106
>>>>
>>>> Modified:
>>>> llvm/trunk/lib/LTO/LTOModule.cpp
>>>> llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>> llvm/trunk/tools/lto/lto.cpp
>>>>
>>>> Modified: llvm/trunk/lib/LTO/LTOModule.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/LTO/LTOModule.cpp?rev=258298&r1=258297&r2=258298&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/LTO/LTOModule.cpp (original)
>>>> +++ llvm/trunk/lib/LTO/LTOModule.cpp Wed Jan 20 03:03:42 2016
>>>> @@ -105,8 +105,10 @@ LTOModule::createFromFile(LLVMContext &C
>>>> TargetOptions options) {
>>>> ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
>>>> MemoryBuffer::getFile(path);
>>>> - if (std::error_code EC = BufferOrErr.getError())
>>>> + if (std::error_code EC = BufferOrErr.getError()) {
>>>> + Context.emitError(EC.message());
>>>> return EC;
>>>> + }
>>>> std::unique_ptr<MemoryBuffer> Buffer = std::move(BufferOrErr.get());
>>>> return makeLTOModule(Buffer->getMemBufferRef(), options, &Context);
>>>> }
>>>> @@ -123,8 +125,10 @@ LTOModule::createFromOpenFileSlice(LLVMC
>>>> off_t offset, TargetOptions options) {
>>>> ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
>>>> MemoryBuffer::getOpenFileSlice(fd, path, map_size, offset);
>>>> - if (std::error_code EC = BufferOrErr.getError())
>>>> + if (std::error_code EC = BufferOrErr.getError()) {
>>>> + Context.emitError(EC.message());
>>>> return EC;
>>>> + }
>>>> std::unique_ptr<MemoryBuffer> Buffer = std::move(BufferOrErr.get());
>>>> return makeLTOModule(Buffer->getMemBufferRef(), options, &Context);
>>>> }
>>>> @@ -158,8 +162,10 @@ parseBitcodeFileImpl(MemoryBufferRef Buf
>>>> // Find the buffer.
>>>> ErrorOr<MemoryBufferRef> MBOrErr =
>>>> IRObjectFile::findBitcodeInMemBuffer(Buffer);
>>>> - if (std::error_code EC = MBOrErr.getError())
>>>> + if (std::error_code EC = MBOrErr.getError()) {
>>>> + Context.emitError(EC.message());
>>>> return EC;
>>>> + }
>>>>
>>>> if (!ShouldBeLazy) {
>>>> // Parse the full file.
>>>>
>>>> Modified: llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto/llvm-lto.cpp?rev=258298&r1=258297&r2=258298&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/llvm-lto/llvm-lto.cpp (original)
>>>> +++ llvm/trunk/tools/llvm-lto/llvm-lto.cpp Wed Jan 20 03:03:42 2016
>>>> @@ -294,7 +294,6 @@ int main(int argc, char **argv) {
>>>> CurrentActivity = "loading file '" + InputFilenames[i] + "'";
>>>> ErrorOr<std::unique_ptr<LTOModule>> ModuleOrErr =
>>>> LTOModule::createFromFile(Context, InputFilenames[i].c_str(), Options);
>>>> - error(ModuleOrErr, "error " + CurrentActivity);
>>>> std::unique_ptr<LTOModule> &Module = *ModuleOrErr;
>>>> CurrentActivity = "";
>>>>
>>>>
>>>> Modified: llvm/trunk/tools/lto/lto.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/lto/lto.cpp?rev=258298&r1=258297&r2=258298&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/tools/lto/lto.cpp (original)
>>>> +++ llvm/trunk/tools/lto/lto.cpp Wed Jan 20 03:03:42 2016
>>>> @@ -81,7 +81,6 @@ static void diagnosticHandler(const Diag
>>>> DiagnosticPrinterRawOStream DP(Stream);
>>>> DI.print(DP);
>>>> }
>>>> - sLastErrorString += '\n';
>>>> }
>>>>
>>>> // Initialize the configured targets if they have not been initialized.
>>>> @@ -111,7 +110,6 @@ namespace {
>>>> static void handleLibLTODiagnostic(lto_codegen_diagnostic_severity_t Severity,
>>>> const char *Msg, void *) {
>>>> sLastErrorString = Msg;
>>>> - sLastErrorString += "\n";
>>>> }
>>>>
>>>> // This derived class owns the native object file. This helps implement the
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list