jeremy-rifkin / cpptrace

Simple, portable, and self-contained stacktrace library for C++11 and newer
MIT License
621 stars 64 forks source link

fix compile error in gcc 4.8.5. #133

Closed hxdnshx closed 3 months ago

hxdnshx commented 3 months ago

I attempted to build cpptrace in a Centos 7 (gcc-c++ 4.8.5) environment using CMake with default parameters and encountered the following compilation errors:

cpptrace/src/symbols/symbols_with_libdwarf.cpp: 47: 24: error: cannot bind 'std::unique_ptr<cpptrace::detail::libdwarf::symbol_resolver>' lvalue to 'std: unique_ptr<cpptrace: detail::libdwarf::symbol_resolver>&&'

return resolver_object;

The code related to this error is:

https://github.com/jeremy-rifkin/cpptrace/blob/06226ee2aa6a615a63af9d73d69e522c10093dd2/src/symbols/symbols_with_libdwarf.cpp#L35-L50

It appears that line 47 transfers ownership of the object to an external entity, but in the aforementioned gcc version, there are errors related to move operations. Therefore, I added this part.

cpptrace/src/symbols/symbols_with_libdwarf. cpp: 64: 17: required from here
/usr/include/c++/4.8.2/bits/stl_iterator.h:963:37: error invalid initialization of reference of type 'std::move_iterator<std::reverse_iterator<__gnu_cxx::__normal_iterator<const cpptrace::stacktrace_frame*, std: vector<cpptrace::stacktrace_frame> >>>::reference aka cpptrace::stacktrace_frame&&' from expression of type 'std::remove_reference<const cpptrace::stacktrace_frame&>::type aka {const cpptrace::stacktrace_frame}'

return std::move(*_M_current); 

The code related to this error is:

https://github.com/jeremy-rifkin/cpptrace/blob/06226ee2aa6a615a63af9d73d69e522c10093dd2/src/symbols/symbols_with_libdwarf.cpp#L53-L64

I'm not sure why it doesn't work properly on the aforementioned gcc version, so I changed the approach. At least it passed the compilation.

In summary, I tried to fix these compilation errors, and finally, it seems to be running normally.

jeremy-rifkin commented 3 months ago

Hi, thanks for taking the time to contribute to the project! Gcc 4.8.5 is really old, however I think it's worth supporting for the project. This is unfortunately an annoying gcc 4 bug with improper handling of automatic moves: https://godbolt.org/z/9oWdWjbf8. Unfortunately return std::move(x); will make later gcc upset over the pessimizing move. The best way to workaround this is probably with explicitly constructing the object being returned: return S{std::move(x)}.

The changes to final_trace.insert you proposed will end up copying frames instead of moving which was the intention though that's not possible due to const auto& entry in the loop above. I think if you change it to auto& entry it'll work.

hxdnshx commented 3 months ago

First issue: As you suggested, I added an explicit construction of the return type to avoid compiler warnings.

For the second issue, the const stacktrace_frame& in the comment is actually a result of type deduction. I tried adding std::make_move_iterator as per the above code reference, as follows:

for(auto iter = std::make_move_iterator(entry.inlines.rbegin()); iter != std::make_move_iterator(entry.inlines.rend()); ++iter){
                    auto&& val = *iter; 
                    final_trace.emplace_back(val);
}

Finally, the same compilation error occurred at auto& val = *iter;.

https://github.com/jeremy-rifkin/cpptrace/blob/06226ee2aa6a615a63af9d73d69e522c10093dd2/src/symbols/symbols_with_libdwarf.cpp#L53

However, note that the parameter in the function signature is const std::vector<frame_with_inlines>& trace, which is a constant reference, and its members should also not be modifiable/movable. Therefore, I haven't thought of a good way to use move semantics here.

jeremy-rifkin commented 3 months ago

Yeah, the const of the stacktrace_frames there is my mistake. It'd be great to fix that in this PR. Could you try the following and see if gcc 4.8.5 finds it amenable:

        std::vector<stacktrace_frame> flatten_inlines(const std::vector<frame_with_inlines>& trace) {
        std::vector<stacktrace_frame> final_trace;
        for(auto& entry : trace) {
            // most recent call first
            if(!entry.inlines.empty()) {
                // insert in reverse order
                final_trace.insert(
                    final_trace.end(),
                    std::make_move_iterator(entry.inlines.rbegin()),
                    std::make_move_iterator(entry.inlines.rend())
                );
            }
            ...
            ...
        }
hxdnshx commented 3 months ago

I tried the code you provided, but it still fails to compile under version 4.8.5. Notably, the change removes the const qualifier before auto. However, for type deduction of auto & in this context, since it is a reference type, the cv qualifier is not ignored. The final deduced type is still const frame_with_inlines&.

jeremy-rifkin commented 3 months ago

Oh sorry yeah, needs to be flatten_inlines(std::vector<frame_with_inlines>& trace)

hxdnshx commented 3 months ago

I reviewed the related code and found that the function is used only at the following location:

https://github.com/jeremy-rifkin/cpptrace/blob/06226ee2aa6a615a63af9d73d69e522c10093dd2/src/symbols/symbols_with_libdwarf.cpp#L138.

It appears that the cv qualifier can be safely removed from the parameters.

After removing the cv qualifier, the original code can be built successfully without any modifications.