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:
I will take a look on this. :-)
Created attachment 88526 [details]
This is the results after ran stack on libreoffice code.
Can we start change the code where stack warned us?
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?
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:
pPipeImpl = (oslPipe)calloc(1, sizeof(struct oslPipeImpl));
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():
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.
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.)
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.
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?
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?
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.
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.
(In reply to comment #5)
> Now, what to do in this particular case then is a question of taste, more or
> 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.
Created attachment 88771 [details]
I executed STACK lint tool again. And finally! We can see some warnings in our codebase.
From attachment 88771 [details]:
> bug: anti-simplify
> model: |
> %68 = icmp eq %"class.configmgr::configuration_registry::<anonymous namespace>::RegistryKey"* %2, null, !dbg !5935
> --> false
> - /home/vanz/git/mylibo/configmgr/source/configurationregistry.cxx:398:0
> ncore: 1
> - /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)
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.
Jose Guilherme Vanz committed a patch related to this issue.
It has been pushed to "master":
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:
Affected users are encouraged to test the fix and report feedback.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup )
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC)