Skip to content

[Bioc-devel] R CMD check without WARNING: g++ and STL issue for xcms and mzR

10 messages · Kasper Daniel Hansen, Vincent Carey, Hervé Pagès +2 more

#
Hi,

in preparation for the release, we are hunting down WARNINGS
"Found ?abort?, possibly from ?abort? (C)" in xcms/mzR.
The abort() call is not coming from XCMS, but rather?
from the C++ code in the STL, and we have no idea?
how to get rid of it.

We are tracking this in: https://github.com/sneumann/xcms/issues/150

In a nutshell, the standard tenplate library (STL) that comes?
with g++-5.4.0 has a snippet:

https://github.com/gcc-mirror/gcc/blob/1cb6c2eb3b8361d850be8e8270c59727
0a1a7967/libstdc%2B%2B-v3/include/bits/stl_list.h#L1825

...
# 1770 "/usr/include/c++/5/bits/stl_list.h"
void _M_check_equal_allocators(list& __x) {
?if (_M_get_Node_allocator())
???__builtin_abort();
}

You can also check this by looking at the compiler output?
after the preprocess step:

g++ -I/usr/share/R/include -DNDEBUG??????-fpic??-g -O2 -fstack-
protector-strong -Wformat -Werror=format-security -Wdate-time
-D_FORTIFY_SOURCE=2 -g??-c massifquant/TrMgr.cpp -E | less

The same applies to mzR, where the same stl_list.h is included
in two files (Found ?abort?, possibly from ?abort? (C)
????Objects: ?./boost/libs/regex/src/instances.o?,
???????./boost/libs/regex/src/winstances.o?)

=> Since we don't want to patch the STL, we have no idea?
???how to fix that WARNING. Is there a way to get an exception,?
???or patch/whitelist this call in R CMD check ?

Yours,?
Steffen Neumann
#
On 04/02/2017 06:52 AM, Neumann, Steffen wrote:
I don't think Bioconductor can help with this; maybe the Rcpp or R-devel 
mailing lists?

Presumably there is a minimal example that generates the warning? It 
would be convenient to communicate that (maybe a package as github 
repo?) rather than the prospect of mzR / xcms installation & debugging.

Martin
This email message may contain legally privileged and/or...{{dropped:2}}
9 days later
#
Hi Martin and malbec2 admin(s):

Some more digging revealed that malbec2?
http://bioconductor.org/checkResults/devel/bioc-LATEST/malbec2-NodeInfo.html
is using "CFLAGS=-g -O2 -Wall" but only "CXXFLAGS=-Wall"
while e.g. tokay2 uses "CXXFLAGS=-O2 -Wall -mtune=core2"

The -O2 optimisation is getting rid of the abort() symbol,
as shown in?https://github.com/sneumann/xcms/issues/150#issuecomment-293545521

=> Is there a way to get -O2 back on the BioC build machines ??
???This should fix our WARNING. Bonus: will fix the same issue?in mzR :-)

Yours,
Steffen
On So, 2017-04-02 at 10:01 -0400, Martin Morgan wrote:
[...]
[...]
--?
3rd Leibniz Plant Biochemistry Symposium, 22.-23. of June 2017
http://www.ipb-halle.de/en/public-relations/3rd-leibniz-plant-biochemis
try-symposium/
--
IPB Halle????????????????????AG Massenspektrometrie &
Bioinformatik
Dr. Steffen Neumann??????????http://www.IPB-Halle.DE
Weinberg 3 ? ? ? ? ? ? ? ? ? Tel. +49 (0) 345 5582 - 1470
06120 Halle ? ? ? ? ? ? ? ? ? ? ? +49 (0) 345 5582 - 0 ? ? ? ? ??
sneumann(at)IPB-Halle.DE?????Fax. +49 (0) 345 5582 - 1409
#
I think "we" have to appreciate that the warning about abort/etc and others
is really hard to deal with when you're including (large) external source
as you do in mzR and for example affxparser / Rgraphviz.  Generally fixing
those in external code requires a lot of effort, and the further effort to
document that the included sources in the package are different from the
official sources.  I have had warnings about this in affxparser for a long,
long time and no-one has bothered me over it.

What might be nice is (somehow) setting a flag saying this should be
ignored in the check process, but of course we don't want that flag to be
set by the developer, because usually, these issues should be dealt with.
So perhaps there is nothing to do and I always have to click on each build
report to verify that there is only 1 warning from this issues and not
others.

Best,
Kasper

On Wed, Apr 12, 2017 at 7:22 AM, Neumann, Steffen <sneumann at ipb-halle.de>
wrote:

  
  
#
Suppose you had a handler for SIGABRT in your code.  Could CMD check check
for that and, if found, refrain from warning?  That is somewhat involved
and goes beyond Bioc but it seems a principled way of dealing with
operations in binary infrastructure whose behavior we cannot control.  The
problem will surely arise in other settings.  Could BiocCheck prototype the
approach?

On Wed, Apr 12, 2017 at 9:12 AM, Kasper Daniel Hansen <
kasperdanielhansen at gmail.com> wrote:

            

  
  
#
Hi,

certainly these are good suggestions, but I would tend?
to simply add some whitelisting functionality if the?
cause is beyond the package's control.?

In this case I doubt it is a?handler for SIGABRT,?
since that would not go away with -O2, or would it ?

For now, just adding -O2 on Linux would fix this issue.?

Yours,
Steffen
On Mi, 2017-04-12 at 10:07 -0400, Vincent Carey wrote:
#
Hi Steffen,
On 04/12/2017 04:22 AM, Neumann, Steffen wrote:
This is apparently the new default for R 3.4 beta. Don't know if that
was an intended change (someone might want to bring this up on R-devel),
but, if it was, then let's assume they had a good reason for it.

So I'm not too keen on touching this on the build machines. Might get
rid of the mzR and xcms warnings, but might also introduce other
problems.

Why not control the compilation flags at the package level?

Thanks,
H.

  
    
#
On 04/12/2017 11:31 AM, Neumann, Steffen wrote:
The code is compiled on the user machine, so twiddling with the build 
system masks the problem from the developer while leaving the user 
vulnerable.

Calling abort() is certainly a serious problem, abruptly ending the user 
session.

Optimization could either eliminate a dead code path, or it could 
compile the call to abort in a way that R no longer recognizes it -- R 
is doing the equivalent of nm *.o |grep abort.

It would be appropriate to ignore the warning about abort() if it were 
never reached; one would only know this by careful code analysis rather 
than adjusting optimization flags.

Rsamtools re-defines abort (before including offending headers), and 
avoids the warning (and crash), with the equivalent of

   PKG_CFLAGS=-D__builtin_abort=_samtools_abort

where _samtools_abort is my own implementation to signal an R error 
telling the user that an unrecoverable error has occurred and they 
should stop what they are doing. Unfortunately this requires 
-U_FORTIFY_SOURCE and doesn't actually address the reason for the abort, 
and is hardly ideal.

I don't think the developer can set the optimization flag in a way that 
overrides the R or user setting (the PKG_CXXFLAGS is inserted before the 
R flags). And it doesn't address the underlying problem anyway.

Kasper's pragmatic approach (it's a very unlikely situation, and 
difficult to fix, so live with the warning) seems like it would often be 
a reasonable one.

FWIW this little bit of C++ seems to be enough to include abort

tmp.cpp:
-------
#include <list>

int foo(int argc, const char *argv[]) {
     std::list<int> l1, l2;
     std::list<int>::iterator it;

     it = l1.begin();
     l1.splice (it, l2); // mylist1: 1 10 20 30 2 3 4

     return 0;
}
-------

Test with

   rm -f tmp.o && R CMD SHLIB tmp.cpp && nm tmp.o | grep abort

with compiler settings in

~/.R/Makevars
-------------
CXXFLAGS = -g -O0
-------------


Martin
This email message may contain legally privileged and/or...{{dropped:2}}
6 days later
#
Hi,

just a quick report, I followed this up to r-devel,
and a fix adding -O2 back to default CXXFLAGS?
has been committed (r72549, see [1]) to R by?Martyn Plummer,?
thus "fixing" the abort() related WARNING in mzR/xcms?
once this gets into the R version used on?malbec2.

Yours,
Steffen

[1]?http://developer.r-project.org/R_svnlog_2013
On Mi, 2017-04-12 at 22:07 -0400, Martin Morgan wrote:
#
Thanks Steffen for bringing this to the R-devel list. I only
see the fix in trunk for now. They'll need to port it to the 3.4
branch if we want to see it propagate to our BioC 3.5 builds.

Just to clarify, by default R doesn't use the -Wall flag to
compile packages on Linux. It's something we add on our Linux
builders.

Cheers,
H.
On 04/19/2017 11:52 AM, Neumann, Steffen wrote: