Opened 11 years ago

Closed 4 years ago

#6126 closed Bugs (fixed)

Signed integer members of Boost.Fusion adapted ADTs are not output correctly with Boost.Spirit.Karma rules

Reported by: t0rt1e@… Owned by: Hartmut Kaiser
Milestone: To Be Determined Component: spirit
Version: Boost 1.48.0 Severity: Regression
Keywords: patch proposed, Boost.Spirit.Karma, Boost.Fusion, BOOST_FUSION_ADAPT_CLASS, BOOST_FUSION_ADAPT_ADT, short, int, long Cc:

Description

Boost.Spirit.Karma output rules should show the same behavior for Boost.Fusion-adapted structs (direct access of the public member variables) and ADTs (access to the private member variables via getters and setters). This has been the case in Boost 1.44 and is demonstrated in this test case for rational number ADTs and structs, which use signed integer member variables. The test case takes into account the name change of macro BOOST_FUSION_ADAPT_CLASS to BOOST_FUSION_ADAPT_ADT as well as the additionally required header boost/spirit/include/support_adapt_adt_attributes.hpp (cf. to ticket #5780).

Compilation of this test case against Boost 1.45.0 and 1.46.1 will fail. Compilation against Boost 1.47.0 and 1.48.0 will succeed, but the output of negative values of signed integer members of Boost.Fusion-adapted ADTs will be wrong. The minus sign is output correctly, but it is followed by a wrong value, which seems to be yielded by a cast from a signed to an unsigned integer value instead of taking the absolute value of the signed integer value. This has been observed on Mac OS X 10.7.2 (x86_64) with Xcode 4.2 using Apple's g++ 4.2.1, Apple's clang++ 3.0. Interestingly, compiling the test case against Boost 1.47.0 or Boost 1.48.0 using MacPorts g++ 4.5.3, will yield more output checks to fail for Boost.Fusion-adapted ADTs used in Karma output rules.

Compilation against Boost from the Subversion trunk (rev. 75505) does only succeed using Apple's clang++ 3.0 and execution will yield the same wrong output for negative values of signed integer members of Boost.Fusion-adapted ADTs. Compilation fails with Apple's g++ 4.2.1 and MacPorts g++ 4.5.3.

Attached to this ticket are the source of the test case, the output of the failing test case compiled against Boost 1.47/1.48 using gcc 4.2.1 and clang++ 3.0 as well as the different output using MacPorts g++ 4.5.3. The g++ 4.2.1 and g++ 4.5.3 compiler outputs for compilation against Boost trunk rev. 75505 are attached as well.

Attachments (9)

test_signed_integer_output_with_karma.cpp (11.6 KB ) - added by t0rt1e@… 11 years ago.
test case
test_signed_integer_output_with_karma_boost148_gcc42_clang3.log (1.0 KB ) - added by t0rt1e@… 11 years ago.
Test case output showing failing checks when compiled against Boost 1.48.0 using Apple's g++ 4.2.1 or clang++ 3.0
test_signed_integer_output_with_karma_boost148_gcc45.log (2.0 KB ) - added by t0rt1e@… 11 years ago.
Test case output showing failing checks when compiled against Boost 1.48.0 using MacPorts g++ 4.5.3
test_signed_integer_output_with_karma_boost_trunk_gcc42.log (109.5 KB ) - added by t0rt1e@… 11 years ago.
Apple's g++ 4.2.1 output failing to compile test case against Boost trunk rev. 75505
test_signed_integer_output_with_karma_boost_trunk_gcc45.log (112.8 KB ) - added by t0rt1e@… 11 years ago.
MacPorts g++ 4.5.3 output failing to compile test case against Boost trunk rev. 75505
test_signed_integer_output_with_karma_minimal.cpp (5.2 KB ) - added by t0rt1e@… 11 years ago.
Minimal test case making not use of Boost.unit_test features to compile cleanly against Boost svn trunk
test_short_output_with_karma.cpp (606 bytes ) - added by bugs@… 11 years ago.
Further reduced test case
karma_numeric_int_hpp.patch (1.8 KB ) - added by t0rt1e@… 11 years ago.
Patch for boost/spirit/home/karma/numeric/int.hpp based on SVN rev. 76398
boost_spirit_home_karma_numeric_int_diagnostic.patch (2.9 KB ) - added by t0rt1e@… 11 years ago.
Diagnostic patch to be able to better debug insert_int

Download all attachments as: .zip

Change History (31)

by t0rt1e@…, 11 years ago

test case

by t0rt1e@…, 11 years ago

Test case output showing failing checks when compiled against Boost 1.48.0 using Apple's g++ 4.2.1 or clang++ 3.0

by t0rt1e@…, 11 years ago

Test case output showing failing checks when compiled against Boost 1.48.0 using MacPorts g++ 4.5.3

by t0rt1e@…, 11 years ago

Apple's g++ 4.2.1 output failing to compile test case against Boost trunk rev. 75505

by t0rt1e@…, 11 years ago

MacPorts g++ 4.5.3 output failing to compile test case against Boost trunk rev. 75505

comment:1 by t0rt1e@…, 11 years ago

I have checked the test case against the current Boost trunk (Subversion revision 75963). It fails still to compile with Apple gcc 4.2.1 or MacPorts g++ 4.5.3 / 4.6.2 and compiles fine with Apple clang++ 3.0 but shows wrong runtime behavior.

by t0rt1e@…, 11 years ago

Minimal test case making not use of Boost.unit_test features to compile cleanly against Boost svn trunk

comment:2 by t0rt1e@…, 11 years ago

The original test case using Boost.unit_test fails to compile against the Boost svn trunk due to an issue related to the usage of enable_if<> in some Boost.unit_test header. Therefore, I simplified the test case to just show the wrong formatting of signed integer ADT members. For details cf. to this thread on the Boost.spirit-general mailing list.

comment:3 by t0rt1e@…, 11 years ago

The original test case using Boost.unit_test fails to compile against the Boost svn trunk due to an issue related to the usage of enable_if<> in some Boost.unit_test header. Therefore, I simplified the test case to just show the wrong formatting of signed integer ADT members. For details cf. to the thread with the same title started on 2011-12-26 on the Boost.spirit-general mailing list. (Sorry, I would've supplied a link, but I was not able get past the captcha presented by the Trac system.)

comment:4 by vexocide@…, 11 years ago

I've managed to reproduce the problem, and it's quite interesting to say the least:

~ Jeroen$ g++ --version
i686-apple-darwin10-g++-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3)

~ Jeroen$ g++ test_signed_integer_output_with_karma_minimal.cpp -o
test -I library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = -771751935/3 (expected: -1/3)
rational_int_adt(-2, 4) = -771751935/2 (expected: -1/2)

~ Jeroen$ g++ test_signed_integer_output_with_karma_minimal.cpp -o
test -O2 -I library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = -1/3 (expected: -1/3)
rational_int_adt(-2, 4) = -1/2 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)


~ Jeroen$ g++-mp-4.4 --version
g++-mp-4.4 (GCC) 4.4.6

~ Jeroen$ g++-mp-4.4 test_signed_integer_output_with_karma_minimal.cpp
-o test -I Documents/Code/library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = -1/1 (expected: -1/3)
rational_int_adt(-2, 4) = -1/1 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)

~ Jeroen$ g++-mp-4.4 test_signed_integer_output_with_karma_minimal.cpp
-o test -O2 -I Documents/Code/library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = -1/3 (expected: -1/3)
rational_int_adt(-2, 4) = -1/2 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)


~ Jeroen$ g++-mp-4.6 --version
g++-mp-4.6 (GCC) 4.6.2

~ Jeroen$ g++-mp-4.6 test_signed_integer_output_with_karma_minimal.cpp
-o test -I library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = -771751935/3 (expected: -1/3)
rational_int_adt(-2, 4) = -771751935/2 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)

~ Jeroen$ g++-mp-4.6 test_signed_integer_output_with_karma_minimal.cpp
-o test -O2 -I library/boost-trunk/
~ Jeroen$ ./test
rational_int_adt(2, -6) = 0/0 (expected: -1/3)
rational_int_adt(-2, 4) = 0/0 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)

Yes, that's four different outputs depending on the compiler version and optimization flags used... Hartmut has tried this using MSVC (not sure which version) and that worked just fine. I've currently broken my clang++ installation thus I can't try that, unfortunately.

I believe that I've found the culprit (a specific temporary) and could provide a patch, but that'll come tomorrow (I'll need to thoroughly test it, for obvious reasons). In the mean time someone should ask the GCC guys what's going on.

comment:5 by t0rt1e@…, 11 years ago

I can help with the clang++ output:

$ clang++ --versionApple clang version 3.0 (tags/Apple/clang-211.12) (based on LLVM 3.0svn)Target: x86_64-apple-darwin11.2.0
Thread model: posix
$ clang++ -o test_signed_integer_output_with_karma_minimal \
test_signed_integer_output_with_karma_minimal.cpp \
-I/Users/maehne/Build/boost-trunk/ maehne@epsilon3:Boost 
$ ./test_signed_integer_output_with_karma_minimal
rational_int_adt(2, -6) = -32767/3 (expected: -1/3)
rational_int_adt(-2, 4) = -32767/2 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)
$ clang++ -O2 -o test_signed_integer_output_with_karma_minimal \
test_signed_integer_output_with_karma_minimal.cpp \
-I/Users/maehne/Build/boost-trunk/
$ ./test_signed_integer_output_with_karma_minimal
rational_int_adt(2, -6) = 0/0 (expected: -1/3)
rational_int_adt(-2, 4) = 0/0 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)

So, also for clang++ the results depend on the optimization settings.

That would be great if you've found the culprit! The compiler/optimization-dependent output also made me think of some undefined behavior to be triggered by the C++ code, but, unfortunately, I'm not familiar enough with the Boost.Fusion and Boost.Spirit codebase to hunt it down. Thanks a lot for investing so much effort to analyze and resolve the bug! If you need help to test the patch, I can try it out on my side with my production code.

comment:6 by bugs@…, 11 years ago

I have greatly reduced your minimal sample (attached), and came up with a 'fix'.

Specific versions, options see at the bottom

It sure looks like there is a compiler bug involved. The only problem I have with this hypothesis is, that it seems against all odds for more than one compiler to have the exact same bug in the same area. So, there might be some intricate case of undefined behaviour lurking, that I cannot for the life of me figure out. Let me show you where the problem happens:

boost_1_48_0/boost/spirit/home/karma/numeric/int.hpp:207

         static bool insert_int(OutputIterator& sink, Attribute const& attr)
         {
             return sign_inserter::call(sink, traits::test_zero(attr)
                       , traits::test_negative(attr), force_sign) &&
                    int_inserter<Radix, CharEncoding, Tag>::call(sink
                       , traits::get_absolute_value(attr));
         }

The problem is that the debugger shows that the value being passed to get_absolute_value is simply '0' (zero). Now with all my reasoning, I can't see why that happens except for a compiler bug.

Now the simplest solution was to change the signature:

         template <typename OutputIterator, typename Attribute>
             static bool insert_int(OutputIterator& sink, Attribute attr)

Out of curiosity I tried some other formulations, and the following variant, surprisingly (!!!) yielded the same problem (wrong output!):

         static bool insert_int(OutputIterator& sink, Attribute const& attr)
         {
            if (!sign_inserter::call(sink, traits::test_zero(attr)
                     , traits::test_negative(attr), force_sign))
                return false;
            Attribute copy(attr);
            return
                 int_inserter<Radix, CharEncoding, Tag>::call(sink
                         , traits::get_absolute_value(copy));
         }

whereas the following works correctly:

         static bool insert_int(OutputIterator& sink, Attribute const& attr)
         {
            Attribute copy(attr);
            if (!sign_inserter::call(sink, traits::test_zero(attr)
                     , traits::test_negative(attr), force_sign))
                return false;
            return
                 int_inserter<Radix, CharEncoding, Tag>::call(sink
                         , traits::get_absolute_value(copy));
         }

This most definitely bears the marks of a compiler bug, since both test_zero and test_negative take all their arguments by value and, besides, attr is a const reference. :-)

Important things to note:

  • all testing done with

o gcc 4.6.1 64bit, boost 1_48_0, flags: -g -O0 -fno-inline o clang++ version 2.9 (tags/RELEASE_29/final) x86_64-pc-linux-gnu with the boost lib and flags

  • valgrind was turning up invalid read warnings:

o "Address 0x7fefffc3e is just below the stack ptr. To suppress,

use: --workaround-gcc296-bugs=yes"

  • by generating a hardcoded (non-adapted) short_ of a specific value (e.g. 123) I could reliably make gcc output, say, -123 instead of the correct value first. This lead me to believe there was uninitialized memory / undefined behavior at play. Though the exact output varied across compilers, the valgrind diagnostics matched and the solution works for both compilers.
  • The output of valgrind is still not clean after adding my solution; there may be other instances of the same problem still! Specificly, a very similar invalid read is reported on line 242 of int.hpp (which has a very similar call pattern/reference parameter type). Someone may want to look into that one too.

comment:7 by bugs@…, 11 years ago

The problem is that the debugger shows that the value being passed to get_absolute_value is simply '0' (zero).

For clarity: this is was the value the debugger turned up when debugging the minimal test case. The actual value is undefined and apparently depends on prior contents of the stack (see notes at the bottom for reasons why I think that is the case).

by bugs@…, 11 years ago

Further reduced test case

in reply to:  6 comment:8 by vexocide@…, 11 years ago

This matches what I've found. My initial patch was http://codepad.org/u6ffbDXT but this isn't fully correct as the type of the value returned by get_absolute_value isn't necessarily Attribute (get_absolute_value may return an unsigned int for an int for example, not quite sure why).

comment:9 by t0rt1e@…, 11 years ago

Keywords: patch proposed added
Severity: ProblemRegression

I have tested Jeroen Habraken's and Seth Heren's patches against test_signed_integer_output_with_karma_minimal.cpp. Like suggested by Seth, copying the attr value in any_int_generator<T, CharEncoding, Tag, Radix, force_sign>::insert_int< OutputIterator, Attribute>(sink, attr) has reliably resolved the problem for me on Mac OS X Lion 10.7.2 when compiling with g++ 4.2.1, 4.5.3, 4.6.2 and clang++ 3.0 version. I kept the structure of the return statement and just replace attr with attr_copy.

I attach a patch against Boost svn rev. 76398.

by t0rt1e@…, 11 years ago

Attachment: karma_numeric_int_hpp.patch added

Patch for boost/spirit/home/karma/numeric/int.hpp based on SVN rev. 76398

comment:10 by t0rt1e@…, 11 years ago

I just tried to compile test_signed_integer_output_with_karma_minimal.cpp with g++ 4.7.0 20120114 (experimental) from MacPorts against Boost trunk (svn rev. 76591). Compilation succeeds, but running the test gives still completely wrong output depending on the chosen optimization level:

$ g++-mp-4.7 -O0 -o test_signed_integer_output_with_karma_minimal test_signed_integer_output_with_karma_minimal.cpp -I.../boost-trunk
$ ./test_signed_integer_output_with_karma_minimal
rational_int_adt(2, -6) = -771751935/3 (expected: -1/3)
rational_int_adt(-2, 4) = -771751935/2 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)
$ g++-mp-4.7 -O1 -o test_signed_integer_output_with_karma_minimal test_signed_integer_output_with_karma_minimal.cpp -I.../boost-trunk
$ ./test_signed_integer_output_with_karma_minimal
rational_int_adt(2, -6) = 0/0 (expected: -1/3)
rational_int_adt(-2, 4) = 0/0 (expected: -1/2)
rational_int_struct(2, -6) = 2/-6 (expected: 2/-6)

Output with optimization -O2 or -O3 is the same as with -O1.

comment:11 by t0rt1e@…, 11 years ago

I tried to dig further into the problem by following Seth Heren's debug into the any_integer_iterator::insert_int() member function at boost/spirit/home/karma/numeric/int.hpp:210 when executing the compiled test_short_output_with_karma.cpp. I could further narrow down the moment, when the *attr value becomes zero by serializing the statements and inlining the sign_inserter::call (see attached diagnostic patch boost_spirit_home_karma_numeric_int_diagnostic.patch against Boost trunk 76795). The *attr value becomes zero after a character is put into the sink, which happens in our case in line boost/spirit/home/karma/numeric/int.hpp:221. Why??? Has anyone an idea? Before everything is OK. I tested it by copying the Attribute via a dummy function into another variable. Here's the relevant code snippet with some comments:

        template <typename OutputIterator, typename Attribute>
        static bool insert_int(OutputIterator& sink, Attribute const& attr)
        {
          // I have serialized the calls to better follow with the debugger what's going on.
          bool is_zero = traits::test_zero(attr);
          bool is_neg = traits::test_negative(attr);
          // Until here everything seems OK, *attr still has its value.
          // I can use, e.g., the attribute in a function call.
          Attribute attr_copy = dummy_function(attr);
//          bool r1 = sign_inserter::call(sink, is_zero
//                      , is_neg, force_sign);
          // After the sign_inserter call, *attr becomes surprisingly zero.
          // Therefore, I inlined the important code parts to output the sign.
          if (is_neg) {
            *sink = '-';
            // After putting the character into the sink, the *attr becomes zero.
            // Why??? This happens consistently unimportant whether it's
            // compiled using g++ or clang++ on Linux or Mac OS X. Is the sink
            // not correctly constructed and leaks memory?
            ++sink;
          } else if (force_sign) {
            if (is_zero) {
              *sink = ' ';              
            } else {
              *sink = '+';           
            }
            ++sink;
          }
          bool r1 = true;
          bool r2 = int_inserter<Radix, CharEncoding, Tag>::call(sink
                      , traits::get_absolute_value(attr));
          return r1 && r2;
//            return sign_inserter::call(sink, traits::test_zero(attr)
//                      , traits::test_negative(attr), force_sign) &&
//                   int_inserter<Radix, CharEncoding, Tag>::call(sink
//                      , traits::get_absolute_value(attr));
        }
      
      // Dummy function for pure diagnostic reasons. It just returns a copy of the attribute.
      template<typename Attribute>
      static Attribute dummy_function(Attribute const& attr) {
        return attr;
      };

I'm not anymore sure that we are facing a compiler bug. Maybe the sink is for some reason not correctly initialized?

by t0rt1e@…, 11 years ago

Diagnostic patch to be able to better debug insert_int

comment:12 by t0rt1e@…, 11 years ago

I just wanted to add that the bug is also present in Boost 1.49.0beta1. As this bug happens with g++ and clang++ on Linux and Mac OS X, it might be worth to at least mention it as a known issue in the documentation if it cannot be resolved until the final 1.49.0 release.

comment:13 by bugs@…, 11 years ago

I tracked it down to extract_from_attribute<>::call<>(attr,ctx,true_type). It will generate an inner typedef of

typedef typename mpl::eval_if<

is_one_element_sequence

, detail::value_at_c<Attribute, 0> , mpl::identity<Attribute const&>

::type type;

which is going to be a reference. This is ok for tuples, adapted structs (typical fusion sequences) but not for ADT's as the accessor function returns a short, which can be bound to a const& but only locally (its lifetime won't be extended outside the scope in which the reference is declared).

Now the way I see it in the code, the sample from the docs[1] (int, int, obj.get_age(), obj.set_age(val)) won't work without undefined behaviour, precisely because it results in extract_from_attribute<>::call<> returning a reference to a local (int in that case).

For the OP, I guess the best way to fix it would be to make the accessors return (const) references: (see attached)

struct XR {

short x;

explicit XR(short num = 0) : x(num) {} const short& getx() const { return x; }

short& getx() { return x; }

void setx(short v) { x = v; }

};

BOOST_FUSION_ADAPT_ADT(XR, (short&, const short&, obj.getx(), obj.setx(val)))

This also removed all the other valgrind messages that were previously unaccounted for.

[1] http://www.boost.org/doc/libs/1_48_0/libs/fusion/doc/html/fusion/adapted/adapt_adt.html

comment:14 by bugs@…, 11 years ago

Thinking aloud:

I suppose the extract_from helper should just return the attribute proxy by value.

I can see how that is undesirable for the general case (non-copyable or copy-expensive attributes), but keep in mind that for adapted ADT's, the adt_attribute_proxy instance can easily be returned by value while still allowing the compiler to generate optimum code:

  • RVO elides the copying,
  • the caller can bind the temporary to a const& in case it needs to access it twice (insert_int) and
  • inlining does the rest

Of the wall: might this (the 'premature' decaying of the proxy to it's exposed attribute type) have something to do with the special casing of single-element-sequences in attribute extraction (decaying of a sequence to it's single element)?

in reply to:  13 ; comment:15 by Hartmut Kaiser, 11 years ago

Replying to bugs@…:

I tracked it down to extract_from_attribute<>::call<>(attr,ctx,true_type). It will generate an inner typedef of

typedef typename mpl::eval_if<

is_one_element_sequence

, detail::value_at_c<Attribute, 0> , mpl::identity<Attribute const&>

::type type;

which is going to be a reference. This is ok for tuples, adapted structs (typical fusion sequences) but not for ADT's as the accessor function returns a short, which can be bound to a const& but only locally (its lifetime won't be extended outside the scope in which the reference is declared).

Now the way I see it in the code, the sample from the docs[1] (int, int, obj.get_age(), obj.set_age(val)) won't work without undefined behaviour, precisely because it results in extract_from_attribute<>::call<> returning a reference to a local (int in that case).

For the OP, I guess the best way to fix it would be to make the accessors return (const) references: (see attached)

struct XR {

short x;

explicit XR(short num = 0) : x(num) {} const short& getx() const { return x; }

short& getx() { return x; }

void setx(short v) { x = v; }

};

BOOST_FUSION_ADAPT_ADT(XR, (short&, const short&, obj.getx(), obj.setx(val)))

This also removed all the other valgrind messages that were previously unaccounted for.

[1] http://www.boost.org/doc/libs/1_48_0/libs/fusion/doc/html/fusion/adapted/adapt_adt.html

That's excellent news! Thanks. That allows to revert the patch above.

in reply to:  15 ; comment:16 by Seth Heeren <bugs@…>, 11 years ago

Replying to hkaiser:

That's excellent news! Thanks. That allows to revert the patch above.

Do you have ideas on how to fix it? My suggestion for the OP is really a workaround:

  • bug: ADT accessors returning by value leads to UB
  • workaround: don't do that, then?

The docs clearly suggest that returning by value from the accessors is ok practice (and that adds value, IMO). However, the code appears to have the assumption that attributes can always be fully 'resolved' to a reference.

Apart from specializing extract_from_attribute for adt_attribute_proxy I don't see an easy way to resolve this. And I wouldn't think that this approach is entirely trivial, either (it might be enough due to the implicit conversion of the proxy to its exposed type?)

in reply to:  16 ; comment:17 by Hartmut Kaiser, 11 years ago

Replying to Seth Heeren <bugs@…>:

Replying to hkaiser:

That's excellent news! Thanks. That allows to revert the patch above.

Do you have ideas on how to fix it? My suggestion for the OP is really a workaround:

  • bug: ADT accessors returning by value leads to UB
  • workaround: don't do that, then?

The docs clearly suggest that returning by value from the accessors is ok practice (and that adds value, IMO). However, the code appears to have the assumption that attributes can always be fully 'resolved' to a reference.

The docs are Fusion related. There it is possible and valid to do such things. The problem is that Karma (falsely) assumes this as well.

Apart from specializing extract_from_attribute for adt_attribute_proxy I don't see an easy way to resolve this. And I wouldn't think that this approach is entirely trivial, either (it might be enough due to the implicit conversion of the proxy to its exposed type?)

Yep, that's what I think needs to be done, at least as a first step.

in reply to:  17 comment:18 by t0rt1e@…, 11 years ago

Thank you very much Seth and Hartmut for finally tracking down the root of the problem! It's good to know that we are facing undefined behavior instead of a compiler bug.

Replying to hkaiser:

Replying to Seth Heeren <bugs@…>:

Replying to hkaiser:

That's excellent news! Thanks. That allows to revert the patch above.

Do you have ideas on how to fix it? My suggestion for the OP is really a workaround:

  • bug: ADT accessors returning by value leads to UB
  • workaround: don't do that, then?

The docs clearly suggest that returning by value from the accessors is ok practice (and that adds value, IMO). However, the code appears to have the assumption that attributes can always be fully 'resolved' to a reference.

I second Seth's opinion that supporting ADTs, which getters return by value gives added value. In my case, I cannot simply change the return type in the signature of the getters. The original test case test_signed_integer_output_with_karma I provided is close to my production code, where I adapted boost::rational<long> for usage in Spirit and Karma rules. Its implementation returns the numerator and denominator by value.

The docs are Fusion related. There it is possible and valid to do such things. The problem is that Karma (falsely) assumes this as well.

Well, at least till Boost 1.48.0, the Karma tutorial demonstrates adapting std::complex<double> for usage with Karma, which real() and imag() member functions also return by value.

Apart from specializing extract_from_attribute for adt_attribute_proxy I don't see an easy way to resolve this. And I wouldn't think that this approach is entirely trivial, either (it might be enough due to the implicit conversion of the proxy to its exposed type?)

Yep, that's what I think needs to be done, at least as a first step.

Could you maybe provide some more instructions how to specialize correctly adt_attribute_proxy for std::rational<long> or std::complex<double>? I think such kind of explanation and some pitfall notes would be very helpful in the Spirit.Karma documentation. I have not yet had the time to actually try myself specializing adt_attribute_proxy, but hope it won't be more effort than implementing my own rational class.

It would be nice if this issue could be resolved in the long term in a more user-friendly way.

Thank you very much for your help!

in reply to:  17 comment:19 by t0rt1e@…, 11 years ago

Replying to hkaiser:

Replying to Seth Heeren <bugs@…>:

Replying to hkaiser:

That's excellent news! Thanks. That allows to revert the patch above.

[...]

Apart from specializing extract_from_attribute for adt_attribute_proxy I don't see an easy way to resolve this. And I wouldn't think that this approach is entirely trivial, either (it might be enough due to the implicit conversion of the proxy to its exposed type?)

Yep, that's what I think needs to be done, at least as a first step.

Since reimplementing the boost::rational getters to return by reference instead by value isn't an option for me, I had a look to the Karma sources to try Seth's suggestion. extract_from_attribute has been already specialized for adt_attribute_proxy in boost/spirit/home/support/adapt_adt_attributes.hpp:192. Commenting out this specialization, will generate a compiler warning:

boost/spirit/home/karma/detail/extract_from.hpp:71:20: warning: returning reference to local temporary object
            return extract_from<Exposed>(fusion::at_c<0>(attr), ctx);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This also happens if the return type typedef is changed to fusion::extension::adt_attribute_proxy<T, N, Const> in boost/spirit/home/support/adapt_adt_attributes.hpp:192 and the proxy object is directly returned call() like suggested by Seth. I tried to further follow the extract_from to see where the proxy gets converted to a const ref to the contained type, but unfortunately got lost. So, I'm stuck again.

in reply to:  15 comment:20 by t0rt1e@…, 11 years ago

Replying to bugs@…:

[snip]

For the OP, I guess the best way to fix it would be to make the accessors return (const) references: (see attached)

struct XR {

short x;

explicit XR(short num = 0) : x(num) {} const short& getx() const { return x; }

short& getx() { return x; }

void setx(short v) { x = v; }

};

BOOST_FUSION_ADAPT_ADT(XR, (short&, const short&, obj.getx(), obj.setx(val)))

This also removed all the other valgrind messages that were previously unaccounted for.

Without any alternative, I went through the pain to reimplement boost::rational<long> for my project so that its getters return const long&. For some reasons that I don't fully understand, the with BOOST_FUSION_ADAPT_ADT() adapted class showed still the same wrong output when used in the Karma rules. I used:

BOOST_FUSION_ADAPT_ADT(
  bufilt::rational,
  (const long&, const long&, obj.numerator(), /**/)
  (const long&, const long&, obj.denominator(), /**/)
)

Then, I tried to use BOOST_FUSION_ADAPT_STRUCT() in the following way:

BOOST_FUSION_ADAPT_STRUCT(
  bufilt::rational,
  (const long&, numerator())
  (const long&, denominator())
)

after remembering having seen a post from someone with a similar problem on the Boost or Spirit mailing list, for which this did the trick. This surprisingly worked! (I just need read access to the struct members for use in the Karma rules.)

So this resolves my urgent problem, but the solution remains a hack that I would like to get rid off again.

comment:21 by Nikita Kniazev <nok.raven@…>, 5 years ago

I am sorry I do not have a time machine, but... Good news here :-)

I opened a PR fixing the bug https://github.com/boostorg/spirit/pull/375.

comment:22 by Joel de Guzman, 4 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.