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

Frédéric Riss via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 10 17:49:31 PST 2016 

> On Feb 8, 2016, at 2:35 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> Is the problem something that can be reproduced with a released ld64?

Sorry it took me so long, but I wanted to understand exactly what’s going on. 

Yes, you can reproduce with a released strip. Take a release mode libLTO.dylib from trunk and replace the one from your toolchain with it (or create sibling bin/ and lib/ directories and put the strip binary in the former and libLTO in the latter). Then run ‘bin/strip <random text file>’, it will fail. The normal behavior is to print a warning but exit with success status.

What is going on is pretty funny. As I mentioned earlier in the thread, strip is testing lto_module_is_object_file_in_memory() before creating the module (which is the failing call). And Petr showed that this function still works. The trick is, strip is calling dlsym to find that function and it had a hand rolled prototype for it:

static int (*lto_is_object)(const void* mem, size_t length) = NULL;

but the actual prototype is

_Bool lto_module_is_object_file_in_memory(const void* mem, size_t length);

ie. the return type is wrong. This call must have been failing for a long time (forever?), but as lto_module_create_in_local_context() was just returning NULL, we never realized that.

(and I agree with Petr's analysis and proposed solution) 

Fred


> 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