These are chat archives for ChaiScript/ChaiScript

6th
Oct 2016
FunMiles
@FunMiles
Oct 06 2016 01:06
@lefticus The chai executable I am using to test with valgrind is running is on x86 standard linux.
FunMiles
@FunMiles
Oct 06 2016 01:14
Here is the log:
Compiler is g++4.9.2
Exact same problem in non threaded.
FunMiles
@FunMiles
Oct 06 2016 01:20
Same result with g++6.2

In other words, doing:

wget https://github.com/ChaiScript/ChaiScript/archive/v5.8.3.tar.gz
tar xf v5.8.3.tar.gz
mkdir build
cd build
cmake ../ChaiScript-5.8.3/
make -j 8

valgrind chai

And entering var a=7 once the prompt appears causes all those valgrind errors.

Sorry about the formatting. I was trying to use equal signs to separate the list of commands I have used from the description text.
Jason Turner
@lefticus
Oct 06 2016 02:20
@FunMiles Yes, a agree that valgrind is giving output like that, but valgrind is incorrect (those are all deep inside of the C++ standard library, I have no control over it). It was the crash you are reporting that I'm concerned about
FunMiles
@FunMiles
Oct 06 2016 04:08
I don't have access to the IBM system anymore, so it is not a concern anymore.
FunMiles
@FunMiles
Oct 06 2016 04:14
But when I look at the output of valgrind, it does not look that it is deep inside of the C++ standard to me.
Though I know that with optimization, inlining can report library code in the caller.
Maybe re-compiling in full debug would show better.
FunMiles
@FunMiles
Oct 06 2016 06:59
Here is with debug turned on:
http://pastebin.com/5S1sNvnK
It is not deep inside the C++ standard lib. I use valgrind on many codes with complex use of C++ standard lib and none of them report errors except in MPI implementations.
FunMiles
@FunMiles
Oct 06 2016 07:12
I see a problem. Valgrind is right:
include/chaiscript/language/chaiscript_eval.hpp:401: mutable std::atomic_uint_fast32_t m_clone_loc;
The standard says of the default constructor for atomics: The default constructor is trivial: no initialization takes place other than zero initialization of static and thread-local objects. std::atomic_init may be used to complete initialization.
This variable involves no static or thread-local object if I am not mistaken. Therefore it remains uninitialized.
FunMiles
@FunMiles
Oct 06 2016 07:20
And it propagates itself down to 6 out of the 9 errors reported by valgrind.
Once I initialize it and the other in the line above, I have only 3 out of 9 errors left. I'll look at the other 3 tomorrow.
FunMiles
@FunMiles
Oct 06 2016 07:34
@lefticus Initializing all the std::atomic variables in chaiscript_eval.hpp removes all the complaints from valgrind.
I am not saying it would fix the crash on the IBM, but it makes the code cleaner. Though you may argue that whatever value those atomic have before the first time initialization, the result would be the same. However relying on such reasoning is risky...
Daniel Guzman
@roig
Oct 06 2016 09:25
Hi @lefticus , I can see that you updated the develop branch, is it ready for a new Release ? 6.0? :+1:
Jason Turner
@lefticus
Oct 06 2016 14:55
@FunMiles very sorry - gut reaction. 99% of the time it's a build error on the user's side. You are correct, I've fixed up the code, I just need to make sure it builds on MSVC. I'll probably tag another release. It's possible that I've been relying on system specific behavior on that code. The atomic caching working is /relatively/ new
Jason Turner
@lefticus
Oct 06 2016 15:02
@roig If you're willing to give it a try, it would be great. It has a lot of major changes. I've been reluctant to actually tag 6.0.0 yet... want to try and fix a few more things first
Daniel Guzman
@roig
Oct 06 2016 15:09
@lefticus Where can we see the changes? Tomorrow I will try to integrate it in our engine to see if it improves the performance.
Daniel Guzman
@roig
Oct 06 2016 15:18
That's great!
Daniel Guzman
@roig
Oct 06 2016 15:29
@lefticus I think that the constructor of the Chaiscript class should be public, no? :smile:
Jason Turner
@lefticus
Oct 06 2016 15:35
\me checks for local changes not yet pushed...
ok, that was a little weird, but pushed up
FunMiles
@FunMiles
Oct 06 2016 16:13
@lefticus Always glad to contribute. I understand your initial gut reaction. ;) In this case, I have a lot of experience and know about the issue between various stdlibs. I also am a big fan of using valgrind for finding issues. Sometimes it will show things that are latent bugs and not what you were initially looking for, like in this case I believe. I will probably never know if the Power 8 issue is related or not, unless I can get access to that machine again.
Jason Turner
@lefticus
Oct 06 2016 16:17
@FunMiles I also run my releases through: cppcheck, address sanitizer, thread sanitizer, clang/msvc/gcc with -Wall -Wextra and -Weverything -pedantic... so I didn't expect valgrind to find anything new
FunMiles
@FunMiles
Oct 06 2016 16:20
Well, missing initializations sometimes are though. For classes, I have really gotten into the habit of doing it in class member declarations which is one of too the little praised features C++11 introduced.
:s/though/tough
Ooops this formatting killed my vi code :smile:
Jason Turner
@lefticus
Oct 06 2016 16:24
Yeah, that's actually an upcoming episode of C++ Weekly (class member initializations)