Make grabbers move-only using unique_ptr

[kunaltyagi]

Grabber should not be copyable based on the signal mechanism used.

• Fixes double free silent bug.
• Finishes the refactoring started in Add Grabber::toggle() method #3615

Methodology

• Add noexcept
• Replace NULL with nullptr
• Use const auto for iterator return type
• Use other functions instead of duplicating code
• Replace normal pointers with unique_pointers and default dtor
• Add compiler specific code to deal with non-conformance wrt default dtor

This should have been the end of the saga, but no. MSVC has a non-conformance and surprisingly a check for shooting yourself in the foot that gcc doesn't have. This needed one more step:

• Bash head into keyboard till a correct patch was created. Time taken: 1 month

Saga Continues

The issue of differing behavior between MSVC and GCC wasn't a single issue, despite the same error message, but was actually 2 different instances, with almost the same diagnostic. The blink-and-miss-it difference in diagnostic was the line number which triggered the error. Anyways, these are the 2 major difference between MSVC and GCC

1. Non-conformance: MSVC tries to copy instead of in-place new in emplace
2. Extra Check: MSVC tries to create copy ctor and copy assignment operator despite nobody using them (technically)

Non conformance

MSVC had an annoying bug which tries to copy despite the chance of a move since it has a throwing move assign operator. This non-conformance was surprisingly easier to handle since the compiler diagnostic pointed to the correct line. I was initially using emplace but then on reading the docs, I realized that I could prevent a dynamic allocation by using try_emplace IFF MSVC was providing the feature with C++14 flag due to it's non-conformance. To be clear, try_emplace is not a C++14 feature, but some compilers make it available (for some reasons), and so I added both methods in order to make the grabber a bit more efficient conditionally.

This needed a deferred allocation for try_emplace (else I went to all the macro magic trouble for no reasons) so that if the key is present, there's not even a peep about heap allocation. To eliminate the spurious dynamic memory allocation, I used an empty struct which could implicitly cast to the map's value_type and perform an allocation in that case. This had the added benefit that the struct was implicitly convertible to the desired type to both emplace and try_emplace. The empty struct is copyable, default constructible, no-throw movable and thus eliminates issues dealing with MSVC's non-conformance.

The Check

This should have been the most straightforward fix, but I was already jaded from looking at the same error message due to non-conformance. I tried to create a MCVE but that didn't help since I didn't have MSVC and had to use Godbolt in order to compile (and let me tell you, MSVC on godbolt is nice, but can't compile much and timesout on including boost or even string). The issue was only on MSVC, not even clang which served as a red-herring to me, making me think it was something to do with MSVC's previous non-conformance.

The check in question is simply put, MSVC being conservative and instantiating the base-class functons when asked for by the Derived classes. In this case, the Derived class has a copy ctor and copy assignment op but the Base class does not. GCC should have thrown a similar error message that it's not possible to instantiate the class (because the copy functions are removed by default (since a member aka std::map with values of std::unique_ptr doesn't have them)). But it didn't. This is really a footgun waiting to shoot since the API claims that the derived class is copy-able, despite the base-class being move only.

I had a wild-goose chase running down conspiracy theories. In the end, I decided to explicitly delete the copy functions (coz MSVC might be stupid) and what-do-you-know, GCC, clang and MSVC all threw much better error messages. Then it was simply a matter of removing copy functions and adding move functions to derived classes. Finally the issue was resolved and I do have to say: good job MSVC. If only you could provide better error messages, this would not have taken almost a month.

Lesson Learnt

Python is Zen 😆 , but yes, explicit is better than implicit.

[kunaltyagi]

Windows doesn't like instantiation of std::map<T, std::unique_ptr<U>> on the CI, and I can't reproduce it on Godbolt. Any ideas?

[SergioRAgostinho]

At first sight, the thing that comes to mind is the fact that unique_ptrs do not support copy operations and the std::map supports both copy ctors and assignment. The moment you instantiate the std::map with explicit types, it will try to compile the copy operations and I can suspect a compilation failure under such situations.

Just throwing guesses at this point. I haven't seen the log but if gcc and clang do not complaint, it tells me I'm overlooking something.

[kunaltyagi]

GCC and Clang do not complain. Actually, even godbolt says MSVC doesn't complain. But for some reason, the CI complains

[kunaltyagi]

I think the issue is that the CI doesn't use C++14 for Windows

[kunaltyagi]

This sounds crazy, but I think this is it, since the issue "can't instantiate" brings up SO questions about pre-"move-semantics" era and I didn't see C++14 being explicitly asked for the Windows CI. Why would Godbolt compile and instantiate and the CI refuse?

@larshg @SunBlack : Any insights?

[SunBlack]

MSVC 2017 does support this construct in general, so maybe it has only trouble with boost im combination.

[larshg]

Seems like @SergioRAgostinho might be on to something. I just tried your PR and I get these compile errors:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\xmemory(671,1): error C2280: 'std::pair<const std::string,std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>>::pair(const std::pair<const std::string,std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>> &)': attempting to reference a deleted function (compiling source file E:\Libaries\pcl\io\src\vlp_grabber.cpp)
2>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\utility(168): message : see declaration of 'std::pair<const std::string,std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>>::pair' (compiling source file E:\Libaries\pcl\io\src\vlp_grabber.cpp)
2>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\utility(168,5): message : 'std::pair<const std::string,std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>>::pair(const std::pair<const std::string,std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>> &)': function was implicitly deleted because a data member invokes a deleted or inaccessible function 'std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>::unique_ptr(const std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>> &)' (compiling source file E:\Libaries\pcl\io\src\vlp_grabber.cpp)
2>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28314\include\memory(1912,5): message : 'std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>>::unique_ptr(const std::unique_ptr<boost::signals2::signal_base,std::default_delete<boost::signals2::signal_base>> &)': function was explicitly deleted (compiling source file E:\Libaries\pcl\io\src\vlp_grabber.cpp)

According to this it might be because the unique pointers can't be copied. And therefore the compiler marks the copy function as deleted.

You could maybe add the copy constructor explicitly, but not sure if it makes sense? At least that is a suggestion I get from reading the documentation.

I compiled it with VS2019 and c++14 enabled.

Found a "explanation" here - not sure if it helps you in any way :P But its more or less beyond my knowledge/experience atm.

[kunaltyagi]

I compiled it with VS2019 and c++14 enabled

Thanks! It's a relief to know that the error is reproducible on non-CI. Time to get a MCRE. Could you try my small snippet and compile that? I wonder why godbolt doesn't complain about the same.

I'd like to know what happens if you

• explicitly delete the copy functions of the grabber class? (in the PCL code)
• use boost and string instead of the fake classes? (in my snippet)

maybe it has only trouble with boost im combination.

Boost combination might be an issue, but that seems a bit unlikely; BUT I'm all for conspiracy theories now 😆

Found a "explanation" here - not sure if it helps you in any way

That was surprising simple to follow. Map asks the underlying container (RB Tree) if there will be an exception in moving the type? MS STL says Ja. Map asks if it has a copy ctor otherwise? STL permits a falsehood here for dealing with incomplete types (nice explanation for this tradeoff here). So Map decides to copy and the fake-ness is out there for all to see.

EDIT: @larshg Could you try the snippet from my godbolt link? If that succeeds that the problem is elsewhere, else we have a mvce

[kunaltyagi]

So, the issue was that compiler was trying to generate a copy ctor when it wasn't possible and deleting the copy ctor and assignment operator resolved the issue (for Windows).

Thanks for putting up with this noise

[taketwo]

Let's keep this unmerged until 1.10.1 is out. I promise I'll get to that soon!

[SergioRAgostinho]

GitHub reports no changes since my last review.

[taketwo]

Kudos for the story in the PR description. Could you please also add a note for users explaining the API break and behavior change?

[kunaltyagi]

Note to users:

Copying Grabbers resulted in double-free and use-after-free. To fix those issues, grabbers are non-copy-able move-only types now.

(That's the extent of noticeable ABI/API/behavior change)