Bug 71043 - Use STACK lint tool to clean code ...
Summary: Use STACK lint tool to clean code ...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.2.0.0.alpha0+ Master
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2013-10-30 11:30 UTC by Michael Meeks
Modified: 2017-02-14 08:58 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
stack_result (2.76 MB, text/plain)
2013-11-02 06:50 UTC, José Guilherme Vanz
Details
stack_result (210.00 KB, application/x-gzip)
2013-11-06 17:30 UTC, José Guilherme Vanz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2013-10-30 11:30:15 UTC
A paper was recently published on a new clang based lint tool to cleanup LibreOffice code; it would be ideal to compile run this tool on LibreOffice - I believe we'd need to use Clang to do that, and chew through the output:

Paper:

http://pdos.csail.mit.edu/~xi/papers/stack-sosp13.pdf

Code:

http://css.csail.mit.edu/stack/

Thanks !
Comment 1 José Guilherme Vanz 2013-11-01 10:06:15 UTC
I will take a look on this. :-)
Comment 2 José Guilherme Vanz 2013-11-02 06:50:38 UTC
Created attachment 88526 [details]
stack_result

This is the results after ran stack on libreoffice code.
Comment 3 José Guilherme Vanz 2013-11-02 08:07:10 UTC
Can we start change the code where stack warned us?
Comment 4 Tor Lillqvist 2013-11-02 08:32:12 UTC
Great! But give people a chance to look in detail at the results first. Actually, the developer mailing list would be best for discussion about this, I think?
Comment 5 Tor Lillqvist 2013-11-02 09:15:39 UTC
Whee, yes, the results looks indeed quite interesting, even if it takes a (short) while to understand what they are trying to say. Just look at the first thing reported, in sal/osl/unx/pipe.c:

in __osl_createPipeImpl(): 

    pPipeImpl = (oslPipe)calloc(1, sizeof(struct oslPipeImpl));
    pPipeImpl->m_nRefCount =1;
    ...
    return pPipeImpl;

Note that there is no check whether calloc() fails. The code immediately dereferences the returned pointer (which might in theory be NULL, in which case the dereferencing will cause a crash of course). The pointer is returned as the function value.

So let's look at the reported call site of __osl_createPipeImpl():

        pAcceptedPipe= __osl_createPipeImpl();

        OSL_ASSERT(pAcceptedPipe);
        if(pAcceptedPipe==NULL)
        {
            close(s);
            return NULL;
        }

Ha! The checks whether pAcceptedPipe is NULL are totally pointless as we have already dereferenced it inside __osl_createPipeImpl(), so it can't be NULL, and the whole if statement will be optimised away by a modern compiler.

Now, what to do in this particular case then is a question of taste, more or less.

Personally I think that if a memory allocation (of a small amount of memory) fails at this place in the code, which as far as I know is invoked mainly (only?) very early when LO is starting, something is very wrong and there is no point in even trying to recover gracefully from the memory allocation failure. So let __osl_createPipeImpl() be as is and remove the pointless checks for NULL return after the calls to it.

But I assume there is an opposite opinion, too, that each and every memory allocation should be checked and the code should do its utmost to fail gracefully... In that case, the check for memory allocation failure should be moved inside __osl_createPipeImpl() right after the call to calloc(), and in case of failure, NULL should be returned immediately. Hopefully then the return value of __osl_createPipeImpl() is checked in each case.
Comment 6 Tor Lillqvist 2013-11-02 09:26:18 UTC
Reading more of the report, it seems truncated and/or a bit useless? Everything else in it is related to external (bundled) sources, not sure if we want to bother with fixing those? And then there are thousands of repeated "LLVM ERROR: Bad DataLayout ctor used.  Tool did not specify a DataLayout to use?" lines at the end. Is our C++ code somehow too pathological for this tool? (That would not be surprising of course... this codebase has been breaking toolchains for a quarter of a century I think;)

Anyway, yay, this is a great start, but for the majority of our codebase it seems that the tool fails, so that needs to be investigated in detail. (By looking at what happens when just one single source file is run through the tool, and either sending a bug report to the STACK maintainers and hoping they can fix it, or figure out what we need to to do work around the problem.)
Comment 7 José Guilherme Vanz 2013-11-02 10:41:01 UTC
Yeap, analyzing the file more calm I saw what you are saying. Actually, I was a little exciting with the stack, so I posted right after ran the tool. =P

About external source I don't know if a good ideia to try fix it. Maybe, send a report for their developers... It's a possibility.

And about LLVM ERROR, I have already send a e-mail for stack maintainers.
Comment 8 José Guilherme Vanz 2013-11-03 01:23:27 UTC
I was looking for the reason the STACK does not works well in our dabatase.

Well, STACK use clang compiler to does its job. So, if we compile libo with clang we can see this error message:

clang: error: unknown argument: '-fno-enforce-eh-specs'

How can I solve the problem? How can I remove the argument when compile if clang?
Comment 9 Julien Nabet 2013-11-03 08:23:26 UTC
José: it seems -fno-enforce-eh-specs is known from clang 3.1, see http://nabble.documentfoundation.org/Problem-with-configure-checks-for-gcc-options-that-clang-does-not-support-tp3933751p3934727.html
What version do you use?
Comment 10 Tor Lillqvist 2013-11-03 08:25:06 UTC
I, and several other people, compile LibreOffice using Clang all the time, both on Linux (where Clang so far is not mainstream, I guess) and on OS X (for OS X itself and cross-compiling for iOS) (where Clang *is* mainstream, no gcc available any more at all). This has been done for over a year at least. So it  isn't as if it would be anything new to compile LibrOffice using Clang.

The configure.ac has logic to decide whether to use the -fno-enforce-eh-specs option or not, and so far it has worked. So either your version of Clang is in some way then odd and the configure.ac tests break down, or you are telling it to use Clang in some wrong way. What does your autogen.input look like? For me, to use Clang (on Linux), I simply add CC=clang and CXX=clang++ lines to autogen.input.
Comment 11 José Guilherme Vanz 2013-11-03 11:04:03 UTC
Yeap, last night I executed autogen again and compiled with clang again. Now, I think is working well. But I'm still investigating why STACK is not working very well in our codebase.
Comment 12 Stephan Bergmann 2013-11-04 08:59:40 UTC
(In reply to comment #5)
> Now, what to do in this particular case then is a question of taste, more or
> less.
> 
> Personally I think that if a memory allocation (of a small amount of memory)
> fails at this place in the code, which as far as I know is invoked mainly
> (only?) very early when LO is starting, something is very wrong and there is
> no point in even trying to recover gracefully from the memory allocation
> failure. So let __osl_createPipeImpl() be as is and remove the pointless
> checks for NULL return after the calls to it.
> 
> But I assume there is an opposite opinion, too, that each and every memory
> allocation should be checked and the code should do its utmost to fail
> gracefully... In that case, the check for memory allocation failure should
> be moved inside __osl_createPipeImpl() right after the call to calloc(), and
> in case of failure, NULL should be returned immediately. Hopefully then the
> return value of __osl_createPipeImpl() is checked in each case.

In this case, where both callers of __osl_createPipeImpl already use "return NULL" to indicate error, I see no disadvantage in cleanly reporting calloc failure instead of relying on SIGSEGV doing the right thing.
Comment 13 José Guilherme Vanz 2013-11-06 17:30:40 UTC
Created attachment 88771 [details]
stack_result

I executed STACK lint tool again. And finally! We can see some warnings in our codebase.
Comment 14 Stephan Bergmann 2013-11-07 09:39:58 UTC
From attachment 88771 [details]:

> bug: anti-simplify
> model: |
>   %68 = icmp eq %"class.configmgr::configuration_registry::<anonymous namespace>::RegistryKey"* %2, null, !dbg !5935
>   -->  false
> stack: 
>   - /home/vanz/git/mylibo/configmgr/source/configurationregistry.cxx:398:0
> ncore: 1
> core: 
>   - /home/vanz/git/mylibo/workdir/unxlngx6.pro/UnoApiHeadersTarget/udkapi/normal/com/sun/star/uno/XInterface.hdl:18:0
>     - null pointer dereference

For me, configmgr/source/configurationregistry.cxx:398 is

>     return new RegistryKey(*this, css::uno::makeAny(access_));

and workdir/unxlngx6.pro/UnoApiHeadersTarget/udkapi/normal/com/sun/star/uno/XInterface.hdl:18 is

> class SAL_NO_VTABLE XInterface

Are they any different for you?

I'm not exactly sure what code this report is supposed to be about.  It might be about the if statement in the implicit call to inline

> template< class interface_type >
> inline Reference< interface_type >::Reference( interface_type * pInterface ) SAL_THROW(())
> {
>     _pInterface = castToXInterface(pInterface);
>     if (_pInterface)
>         _pInterface->acquire();
> }

which is redundant in this particular case where pInterace is non-null "new RegistryKey(...)" and castToXInterface is an inline function that transfers non-null pointer to non-null pointer.  But that would mean that STACK is unhelpfully too eager in reporting findings in inlined codes.
Comment 15 Commit Notification 2014-03-01 16:01:51 UTC
Jose Guilherme Vanz committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=f6141d884b7a89a856a34dc93991dbe01ded0b6b

fdo#71043 -  Use STACK lint tool to clean code



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 16 Robinson Tryon (qubit) 2015-12-14 06:34:20 UTC Comment hidden (obsolete)
Comment 17 Robinson Tryon (qubit) 2016-02-18 14:52:34 UTC Comment hidden (obsolete)