gcc 5: test_eterm failures

[matwey]

Hello, I am using g++ 5.2.1 on x86_64 and see the following:

> ./test_eterm 
Running 63 test cases...
test_eterm.cpp(351): fatal error: in "test_pid": critical check atom("abc@fc12") == et.node() has failed [abc@fc12 != abc@fc12]
test_eterm.cpp(413): fatal error: in "test_port": critical check atom("abc@fc12") == et.node() has failed [abc@fc12 != abc@fc12]
test_eterm.cpp(441): fatal error: in "test_ref": critical check atom("abc@fc12") == et.node() has failed [abc@fc12 != abc@fc12]
test_eterm_encode.cpp(162): fatal error: in "test_encode_pid": critical check eterm(pid) == t has failed [#Pid<test@host.1.2.0> != #Pid<test@host.1.2.0>]
test_eterm_encode.cpp(185): fatal error: in "test_encode_port": critical check t1 == t has failed [#Port<test@host.1> != #Port<test@host.1>]
test_eterm_encode.cpp(200): fatal error: in "test_encode_ref": critical check eterm(t1) == t has failed [#Ref<test@host.1.2.3> != #Ref<test@host.1.2.3>]
test_eterm_encode.cpp(256): fatal error: in "test_encode_trace": critical check self == t1.from() has failed
test_eterm_match.cpp(68): fatal error: in "test_match1": critical check res has failed
test_eterm_match.cpp(158): fatal error: in "test_match2": critical check m0 has failed
test_eterm_match.cpp(212): fatal error: in "test_match3": critical check atom("x12") == n->to_atom() has failed [x12 != x12]

[saleyn]

I am unable to reproduce this:

$ uname -a Linux nova 4.2.5-1-ARCH #1 SMP PREEMPT Tue Oct 27 08:13:28 CET 2015 x86_64 GNU/Linux [serge@nova ~/projects/eixx]$ gcc --version gcc (GCC) 5.2.0

$ test/test_eterm Running 63 test cases...

*\ No errors detected

[matwey]

I use boost 1.59 and erlang 18.1.3, if it matters

[matwey]

Why don't you just use boost.multiindex for implementing basic_atom_table?

[matwey]

0 
1 _
2 badrpc
3 call
4 cast
5 erlang
6 error
7 false
8 format
9 $gen_cast
10 io_lib
11 latin1
12 noconnection
13 noproc
14 normal
15 ok
16 request
17 rex
18 rpc
19 true
20 undefined
21 unsupported
22 user
23 _
24 _
25 _
26 _
27 _
28 _
29 _
30 _
31 Abc
32 aBc
33 abc
34 abc
35 efg
36 abc
37 abc@fc12
38 abc@fc12

Some atoms are registered twice for some reason.

[matwey]

m_index is also full of duplicates:

0x7ffeebc437e0 38
0x7ffeebc43790 37
0x7ffeebc43510 35
0x7ffeebc43da0 36
0x7ffeebc43510 34
0x7ffeebc43720 33
0x7ffeebc43740 32
0x7ffeebc43740 31
0x7ffeebc45660 22
0x7ffeebc45660 21
0x7ffeebc45660 20
0x7ffeebc45660 19
0x7ffeebc45660 18
0x7ffeebc45660 17
0x7ffeebc45660 16
0x7ffeebc45660 15
0x7ffeebc45660 14
0x7ffeebc45660 13
0x7ffeebc45660 12
0x7ffeebc45660 11
0x7ffeebc45660 10
0x7ffeebc45660 9
0x7ffeebc45660 8
0x7ffeebc45660 7
0x7ffeebc45660 6
0x7ffeebc45660 5
0x7ffeebc45660 4
0x7ffeebc45660 3
0x7ffeebc45660 2
0x7ffeebc45570 30
0x7ffeebc45520 29
0x7ffeebc45530 28
0x7ffeebc454b0 27
0x7ffeebc45370 26
0x7ffeebc452f0 25
0x7ffeebc45270 24
0x7ffeebc45030 23
0x7ffeebc45660 1
0x4d87fa 0

[matwey]

I am sorry, but I can't understand how it is supposed to work.

        int lookup(const String& a_name)
            throw(std::runtime_error, err_bad_argument)
        {
            if (a_name.size() == 0)
                return 0;
            if (a_name.size() > MAXATOMLEN)
                throw err_bad_argument("Atom size is too long!");
            size_t bucket = m_index.bucket(a_name.c_str());
            int n = find_value(bucket, a_name.c_str());
            if (n >= 0)
                return n;

            lock_guard<Mutex> guard(m_lock);
            n = find_value(bucket, a_name.c_str());
            if (n >= 0)
                return n;

            n = m_atoms.size();
            if ((size_t)(n+1) == m_atoms.capacity())
                throw std::runtime_error("Atom hash table is full!");
            m_atoms.push_back(a_name);
            m_index[a_name.c_str()] = n;
            return n;
        }

As far as I know, it is not guaranteed in general that the pointer returned by c_str() remains valid, but in the following code it is obviously assumed in strcmp(a_atom, lit->first):

        int find_value(size_t bucket, const char* a_atom) {
            typename HashMap::const_local_iterator
                lit = m_index.begin(bucket), lend = m_index.end(bucket);
            while(lit != lend && strcmp(a_atom, lit->first) != 0) ++lit;
            return lit == lend ? -1 : lit->second;
        }

[saleyn]

Once created, the size of atom table is not changeable. All atoms stored in the atom table are not relocatable (they are stored in the basic_atom_table::m_atoms vector), and therefore each atom's c_str() is immutable, and can be stored in the hash table as a char pointer.

The use of this implementation was chosen as opposed to multimap for performance in concurrent applications.

Under no circumstances though there should be duplicates in the atom table. I found and fixed the bug that caused your issue. Please check again. Though it's strange that in my case the error wasn't reproducible to begin with.

[matwey]

But what are you saying assumes that the following:

            m_atoms.push_back(a_name);
            m_index[a_name.c_str()] = n;

must be:

            m_atoms.push_back(a_name);
            m_index[m_atoms.back().c_str()] = n;

I mean

All atoms stored in the atom table are not relocatable (they are stored in the basic_atom_table::m_atoms vector), and therefore each atom's c_str() is immutable

It could be true in general, but you don't use c_str() of the atom stored in m_index, use use a_name.c_str() where a_name is transient object.

[saleyn]

Indeed. I found the same and committed a fix a few minutes ago.

[matwey]

Thanks, this fixed ./test_eterm for me.