[llvm] r258298 - [LTO] Fix error reporting when a file passed to libLTO is invalid or non-existent

Rafael Espíndola via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 8 14:35:38 PST 2016


Is the problem something that can be reproduced with a released ld64?

Probably the case that needs to be fixed is makeLTOModule when passed
a nullptr for context. It should probably be split in two. One takes a
context reference. The other takes a diag handler, creates a local
context that uses that handler and passes to the second one.

Cheers,
Rafael


On 8 February 2016 at 11:27, Mehdi Amini via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>> On Feb 8, 2016, at 8:02 AM, Frédéric Riss via llvm-commits <llvm-commits at lists.llvm.org> 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?
>
> Here is the API contract from the libLTO header (TL DR: it should return null, not exit):
>
> /**
> * \brief Loads an object file in its own context.
> *
> * Loads an object file in its own LLVMContext.  This function call is
> * thread-safe.  However, modules created this way should not be merged into an
> * lto_code_gen_t using \a lto_codegen_add_module().
> *
> * Returns NULL on error (check lto_get_error_message() for details).
> *
> * \since LTO_API_VERSION=11
> */
>
> --
> Mehdi
>
>
>
>>
>> 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
>>>>
>>>
>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> 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