Hi, On 2021-03-22 11:20:37 +1300, Thomas Munro wrote: > On Mon, Mar 22, 2021 at 10:50 AM Peter Smith <smithpb2...@gmail.com> wrote: > > The real problem isn't the Assert. It's all those other usages of ent > > disobeying the API rule: "(NB: in the case of the REMOVE action, the > > result is a dangling pointer that shouldn't be dereferenced!)"
Right that's clearly not ok. > I suppose the HASH_REMOVE case could clobber the object with 0x7f if > CLOBBER_FREED_MEMORY is defined (typically assertion builds) Yea. Plus VALGRIND_MAKE_MEM_NOACCESS() (requiring a VALGRIND_MAKE_MEM_UNDEFINED() when reusing) or at least a VALGRIND_MAKE_MEM_UNDEFINED(). >or alternatively return some other non-NULL but poisoned pointer, so >that problems of this ilk blow up in early testing. IMO it's just a bad API to combine the different use cases into hash_search(). It's kinda defensible to have FIND/ENTER/ENTER_NULL go through the same function (although I do think it makes code harder to read), but having HASH_REMOVE is just wrong. The only reason for returning a dangling pointer is that that's the obvious way to check if something was found. If they weren't combined we could tell newer compilers that the memory shouldn't be accessed after the HASH_REMOVE anymore. And it'd remove some unnecessary branches in performance critical code... But I guess making dynahash not terrible from that POV basically means replacing all of dynahash. Having all those branches for partitioned hashes, actions are really not great. Greetings, Andres Freund
