Maybe uninitialized variables when building with GCC in release mode about njoy21

[jlconlin]

We can do that specifically on NJOY21 I think by doing this in NJOY21.hpp:

 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
 #include "lipservice.hpp"
 #pragma GCC diagnostic pop

I wonder if that will be sufficient or if we need to wrap the pragmas around more of the code in NJOY21.hpp.

[jlconlin]

Note, this is likely an issue in the component lipservice than in NJOY21 directly.

[jlconlin]

I think this is a bug in GCC. The code that GCC points to as being uninitialized is here:

class Card12{
public:

using Aw = POWR::Lib3::Card9::Aw;
#include "lipservice/POWR/Lib3/Card12/Temp.hpp"
#include "lipservice/POWR/Lib3/Card12/Fpa.hpp"

Argument< Aw > aw;
Argument< Temp > temp;
Argument< Fpa > fpa;

template< typename Istream >
Card12( Istream& is, const size_t ngnd )
try:
  aw( argument::extract< POWR::Lib3::Card12::Aw >( is ) ),
  temp( argument::extract< POWR::Lib3::Card12::Temp >( is ) ),
  fpa( argument::extract< POWR::Lib3::Card12::Fpa >( is, ngnd ) )
{
  Card::clear( is );
}
catch( std::exception& e ){
  Log::info( "Trouble reading Card 12." );
  throw e;
}
};

You can see that aw, temp, and fpa are all initialized in the constructor.

By the way, when I use GCC version 9.2.0, I get a different uninitialized error.

[nathangibson14]

I got uninitialized errors with both 9.2 and 8.3 yesterday, although I didn't pay attention to if they pointed to the same spots or not.

These "may be uninitialized" errors are actually warnings converted to errors by using strict compiling. We can probably add some compiler flags to GCC to ignore these warnings if we don't find a better solution.

I'll keep looking to see if I can figure anything else out.

[whaeck]

There's probably a reason to this. We didn't have these before so something is going on. I only use 7.2 for the moment but I haven't recompiled lipservice or NJOY21 for a while so I can't say if 7.2 has the same issue.

…

________________________________ From: Nathan A. Gibson <[email protected]> Sent: Wednesday, March 18, 2020 8:27:00 AM To: njoy/NJOY21 Cc: Subscribed Subject: [EXTERNAL] Re: [njoy/NJOY21] Maybe uninitialized variables when building with GCC in release mode (#106) I got uninitialized errors with both 9.2 and 8.3 yesterday, although I didn't pay attention to if they pointed to the same spots or not. These "may be uninitialized" errors are actually warnings converted to errors by using strict compiling. We can probably add some compiler flags to GCC to ignore these warnings if we don't find a better solution. I'll keep looking to see if I can figure anything else out. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#106 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AENOHQB4QFGOFJJJGDDDCK3RIDK3JANCNFSM4LORKKAQ>.

[jlconlin]

We intentionally turned warnings into errors with the hopes that we get better code. I think in general it's a good idea.

We can rethink this policy, or turn off some warnings, or even do it differently from one compiler to another.

[nathangibson14]

Here's a good read for some others with similar code throwing the -Wmaybe-uninitialized warning. I particularly liked the comment "maybe-uninitialized includes maybe for a reason".
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635

This at least relates to the warning being around some std::optional, which is where GCC 9.2 and 8.3 seem to throw the warning. The example @jlconlin included above at least doesn't obviously invoke std::optional, but I suspect it's still the same underlying issue.

I am leaning toward suppressing the -Wmaybe-uninitialized warnings with GCC compiler flags at this point, personally.

[nathangibson14]

Assuming this works, I'm fine with this change. Fortunately, lipservice isn't used by other components.

The other option is to do this in CMake, and attach is specifically to the release build option. That way, warnings would still appear if compiled with debug options of blank or debug.

[whaeck]

We should investigate this further before trying to switch off the error to remove it. Once we start doing this, it'll be always easier to remove the error instead of addressing it. I have no objection doing this on code that we did not write (e.g. we actually have had to do this with Eigen) but we own this code so we should do more to try to figure it out first.

We could assign the values a default initialisation value when declaring them.

struct test {
  int a = 0;
};

[jlconlin]

@whaeck I aree with your sentiment. However, GCC seems to be inconsistent here. It fails in different spots using different versions of GCC. It does not fail when compiling lipservice on its own. Since lip service is tested independently (and this warning/error doesn't come up), I'm comfortable turning off the warning in NJOY21.

[nathangibson14]

I was typing up a similar response to @whaeck as @jlconlin just posted. I don't want to waste a lot of time chasing down what definitely seems to be a GCC bug, particularly with it hitting inconsistently. Redundant declarations can probably avoid this warning, but I'm worried that we'd need a whole lot of these. And lipservice isn't the focus of development now anyway.

[jlconlin]

This problem has cropped up again.

I tried to compile with GCC version 9.2.0 and got this error:

$ cmake -D CMAKE_BUILD_TYPE=Release ..
...
$ make
...
[ 86%] Building CXX object CMakeFiles/njoy21.dir/src/main.cpp.o
In file included from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/DTFR/Card8.hpp:3,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/DTFR.hpp:11,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice.hpp:42,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/src/njoy21.hpp:21,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/src/main.cpp:1:
/Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/DTFR/Card8/Hisnam.hpp: In function 'auto njoy::njoy21::lipservice::argument::extract(njoy::njoy21::lipservice::iRecordStream<Char>&, Args&& ...) [with Policy = njoy::njoy21::lipservice::DTFR::Card8::Hisnam; Char = char; Args = {}]':
/Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/DTFR/Card8/Hisnam.hpp:15:56: error: '*((void*)<anonymous>+8)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   15 |   static bool verify( Value_t str ){ return str->length() <= 6; }
      |                                             ~~~~~~~~~~~^~
In file included from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/LEAPR/Card20.hpp:3,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/LEAPR.hpp:22,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice.hpp:45,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/src/njoy21.hpp:21,
                 from /Users/jlconlin/NJOY21/Code/NJOY21/src/main.cpp:1:
/Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/LEAPR/Card20/Comment.hpp: In function 'auto njoy::njoy21::lipservice::argument::extract(njoy::njoy21::lipservice::iRecordStream<Char>&, Args&& ...) [with Policy = njoy::njoy21::lipservice::LEAPR::Card20::Comment; Char = char; Args = {}]':
/Users/jlconlin/NJOY21/Code/NJOY21/bin-llvm/_deps/lipservice-src/src/lipservice/LEAPR/Card20/Comment.hpp:16:56: error: '*((void*)<anonymous>+8)' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   16 |   static bool verify( Value_t str ){ return str->length() <= 66; }
      |                                             ~~~~~~~~~~~^~

I removed the pragma code in version 1.2.0, and that seems to cause GCC to fail to compile in Release mode.

[staleyLANL]

I think this is a bug in GCC. The code that GCC points to as being uninitialized is here:

class Card12{
public:

using Aw = POWR::Lib3::Card9::Aw;
#include "lipservice/POWR/Lib3/Card12/Temp.hpp"
#include "lipservice/POWR/Lib3/Card12/Fpa.hpp"

Argument< Aw > aw;
Argument< Temp > temp;
Argument< Fpa > fpa;

template< typename Istream >
Card12( Istream& is, const size_t ngnd )
try:
  aw( argument::extract< POWR::Lib3::Card12::Aw >( is ) ),
  temp( argument::extract< POWR::Lib3::Card12::Temp >( is ) ),
  fpa( argument::extract< POWR::Lib3::Card12::Fpa >( is, ngnd ) )
{
  Card::clear( is );
}
catch( std::exception& e ){
  Log::info( "Trouble reading Card 12." );
  throw e;
}
};

You can see that aw, temp, and fpa are all initialized in the constructor.

By the way, when I use GCC version 9.2.0, I get a different uninitialized error.

Just a quick thought, looking at this for the first time. (Maybe you're way ahead of me on this.) Because aw, temp, and fpa are initialized in a "try", the compiler might be thinking, "well, if an exception is thrown while trying, some or all of those might not have been initialized yet." Hence the warning/error.

Of course, anything - anywhere - might not be initialized as it appears to be initialized, if an exception is thrown. I just wonder if the compiler makes an extra fuss about it here, in this (not too commonly used) variation of a try. (During initialization in a constructor.)

I haven't looked closely at the code, but wonder if those could be moved to the constructor's body, rather than initialized in the customary constructor way, without breaking anything. Would the diagnostic go away?

[jlconlin]

@staleyLANL Thanks for looking in to this. We have this construct throughout lipservice; if we fix it for this particular case, it would crop up somewhere else.

I don't think we can move the initialization to the body of the constructor because the Argument<...> class doesn't have default constructors (I think).