util: Use void* throughout support/lockedpool.h #16195

1. 
   jkczyz commented at 3:46 PM on June 12, 2019: contributor

   Replace uses of char* with void* in Arena's member variables. Instead, cast to char* where needed in the implementation.

   Certain compiler environments disallow std::hash<char> specializations to prevent hashing the pointer's value instead of the string contents. Thus, compilation fails when std::unordered_map is keyed by char.

   Explicitly using void* is a workaround in such environments. For consistency, void* is used throughout all member variables similarly to the public interface.

   Changes to this code are covered by src/test/allocator_tests.cpp.

2. in src/support/lockedpool.cpp:104 in 018c7d3804 outdated

    101 | +    auto i = chunks_used.find(ptr);
    102 |      if (i == chunks_used.end()) {
    103 |          throw std::runtime_error("Arena: invalid or double free");
    104 |      }
    105 | -    std::pair<char*, size_t> freed = *i;
    106 | +    std::pair<char*, size_t> freed = std::make_pair(static_cast<char*>(i->first), i->second);
   

   ---

   ---

   

   promag commented at 3:54 PM on June 12, 2019:

   nit,

   std::pair<char*, size_t> freed{static_cast<char*>(i->first), i->second};
   

   or

   auto freed = std::make_pair(static_cast<char*>(i->first), i->second);
   

3. in src/support/lockedpool.cpp:75 in 018c7d3804 outdated

     71 | @@ -71,20 +72,21 @@ void* Arena::alloc(size_t size)
     72 |  
     73 |      // Create the used-chunk, taking its space from the end of the free-chunk
     74 |      const size_t size_remaining = size_ptr_it->first - size;
     75 | -    auto allocated = chunks_used.emplace(size_ptr_it->second + size_remaining, size).first;
     76 | -    chunks_free_end.erase(size_ptr_it->second + size_ptr_it->first);
     77 | +    char* free_chunk = static_cast<char*>(size_ptr_it->second);
   

   ---

   ---

   

   promag commented at 3:55 PM on June 12, 2019:

   make free_chunk const?

4. 
   promag commented at 3:57 PM on June 12, 2019: member

   ACK 018c7d3, code change looks good, just 2 comments for your consideration.

5. 
   practicalswift commented at 4:13 PM on June 12, 2019: contributor

   @jkczyz Nice first-time contribution! Welcome as a contributor!

   What compiler is it that is failing to compile?

6. fanquake added the label Utils/log/libs on Jun 12, 2019
7. 
   jkczyz commented at 5:14 PM on June 12, 2019: contributor

   @practicalswift Thanks! My organization uses a customized STL. It provides a char* specialization for std::hash, which contains a static_assert that always fails. This is to prevent code from keying by pointers when keying by the pointed-to string is what is probably desired.

8. 
   practicalswift commented at 8:55 PM on June 12, 2019: contributor

   @jkczyz Thanks for the clarification! Please squash into one commit.

   Please report any other issues or warnings you encounter when using your organisations' compiler environment. Diversity in testing is good :-)

9. 
   DrahtBot commented at 9:12 PM on June 12, 2019: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack, jonatack, achow101
   Concept ACK	laanwj
   Stale ACK	promag

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

10. jkczyz force-pushed on Jun 12, 2019
11. 
    laanwj commented at 10:47 AM on June 13, 2019: member

    Concept ACK @sipa can you please comment on this? you seem to have a good grasp around subtle issues with regard to UB around pointer casts

12. fanquake requested review from sipa on Jul 30, 2019
13. 
    sipa commented at 6:14 AM on July 30, 2019: member

    My understanding is that casting something to void* and then back to the correct pointer type before use is always fine.

    Will review the code soon.

14. 
    fanquake commented at 2:56 PM on November 20, 2019: member

    @jkczyz Can you rebase this. @sipa Could you follow up after that?

15. DrahtBot added the label Needs rebase on Nov 20, 2019
16. jkczyz force-pushed on Nov 21, 2019
17. Use void* throughout support/lockedpool.h

    Replace uses of char* with void* in Arena's member variables. Instead,
    cast to char* where needed in the implementation.
    
    Certain compiler environments disallow std::hash<char*> specializations
    to prevent hashing the pointer's value instead of the string contents.
    Thus, compilation fails when std::unordered_map is keyed by char*.
    
    Explicitly using void* is a workaround in such environments. For
    consistency, void* is used throughout all member variables similarly to
    the public interface.

    f36d1d5b89
18. jkczyz force-pushed on Nov 21, 2019
19. 
    jkczyz commented at 2:30 AM on November 21, 2019: contributor

    @jkczyz Can you rebase this. @sipa Could you follow up after that?

    Done.

20. DrahtBot removed the label Needs rebase on Nov 21, 2019
21. theStack approved
22. 
    theStack commented at 1:59 PM on December 19, 2021: contributor

    Code-review ACK f36d1d5b8934aac60d3097047ecedeb58bae2185

    The member variables of the Arena class are changed from char* to void* and casts in the code to char* are done whenever pointer arithmetic is involved (which wouldn't work with void*). LGTM.

    My understanding is that casting something to void* and then back to the correct pointer type before use is always fine.

    That would also be my understanding.

23. 
    jonatack commented at 12:24 PM on July 30, 2022: contributor

    ACK f36d1d5b8934aac60d3097047ecedeb58bae2185 review, debug build, unit tests, checked clang 15 raises "error: arithmetic on a pointer to void" without the conversions here from the generic void* pointer back to char*

    some references

    • https://bytes.com/topic/c/answers/618725-void-vs-char
    • https://stackoverflow.com/questions/10058234/void-vs-char-pointer-arithmetic
24. 
    achow101 commented at 8:52 PM on February 3, 2023: member

    This PR has had a couple of acks for a while now and could be ready to merge. Is this change still relevant and valid?

25. 
    achow101 commented at 8:32 PM on February 23, 2023: member

    ACK f36d1d5b8934aac60d3097047ecedeb58bae2185

26. DrahtBot requested review from promag on Feb 23, 2023
27. 
    jkczyz commented at 8:44 PM on February 23, 2023: contributor

    This PR has had a couple of acks for a while now and could be ready to merge. Is this change still relevant and valid?

    I'm not longer employed with the organization in questions, FWIW. This patch was required to compile in their environment as noted in #16195 (comment).

28. achow101 merged this on Feb 23, 2023
29. achow101 closed this on Feb 23, 2023

    ---

30. sidhujag referenced this in commit 7267634706 on Feb 25, 2023
31. bitcoin locked this on Feb 23, 2024

Review Requested
sipapromag