Re: Bug in base: NSMapGet()

[Fred Kiefer]

Thank you Daniel,

I think that this time you found a real issue. It surely would help Richard to 
understand this better if you could provide your new test code. My expectation 
is that you just use an object as the value now.
You should always try to make it as easy as possible for the project maintainer 
to reproduce your issue, even if already provide a fix. The best thing to do 
here is to write a test case that could be added to base/Tests/base/NSMapTable.

Fred


> Am 04.06.2017 um 07:26 schrieb Daniel Ferreira (theiostream) <address@hidden>:
> 
> Okay, I'm ashamed of how long and how many experiments it took me to
> finally see this, and I'm sorry for the misleading example I sent on
> the first thread e-mail that was supposed to go wrong anyway.
> 
> The issue whose fault is Base's is that despite the key barriers being
> set to (NSPointerFunctionsOpaqueMemory |
> NSPointerFunctionsOpaquePersonality), i.e. with nothing to do with
> ARC, Base's NSConcreteMapTable was still calling objc_loadWeak() to
> get the key value.
> 
> After looking at the wrong place for hours, I finally noticed that in
> GSI_MAP_NODE_IS_EMPTY we were reading the table nodes' key values
> using the GSI_MAP_READ_VALUE macro, which applies the *value* barriers
> to whatever it's reading. This way, we were applying value barriers to
> reading the keys, and since the value barriers used
> NSPointerFunctionsWeakMemory, it ended up calling ARC functions on the
> raw pointers that made up the keys causing all sorts of unexpected
> behavior.
> 
> Here's the diff of how I fixed it. (Since how we submit code
> contributions is apparently unstable with the change to Git, I suppose
> I will temporarily send it here for comments, since it's a small one
> anyway.)
> 
> diff --git a/Headers/GNUstepBase/GSIMap.h b/Headers/GNUstepBase/GSIMap.h
> index 47fc8d3..b0ac494 100644
> --- a/Headers/GNUstepBase/GSIMap.h
> +++ b/Headers/GNUstepBase/GSIMap.h
> @@ -157,9 +157,9 @@ extern "C" {
> #  define GSI_MAP_WRITE_VAL(M, addr, obj) (*(addr) = obj)
> #endif
> #if    GSI_MAP_HAS_VALUE
> -#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_VALUE(M,
> &node->key).addr) == 0) || ((GSI_MAP_READ_VALUE(M, &node->value).addr
> == 0)))
> +#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_KEY(M,
> &node->key).addr) == 0) || ((GSI_MAP_READ_VALUE(M, &node->value).addr
> == 0)))
> #else
> -#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_VALUE(M,
> &node->key).addr) == 0))
> +#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_KEY(M,
> &node->key).addr) == 0))
> #endif
> 
> -- Daniel.