Re: [Flightgear-devel] [Flightgear-commitlogs] [SimGear] branch next updated: props: Over-protect nodeOriginsLock

[Julian S. <ju...@op...>]

Apologies if i've misunderstood, but this patch seems a little
questionable - it seems to be hiding a problem rather than trying to
gather information that could be used to fix it.

Could you give some more details of the failure mode we are trying to
cope with here?

[If the problem is that properties are being manipulated after the
static global `std::shared_mutex nodeOriginsLock` is being destructed by
the C++ runtime, and for some reason we cannot avoid this happening,
then maybe we could put nodeOriginsLock on the heap and never delete
it, e.g. with `static std::shared_mutex& nodeOriginsLock = *new
std::shared_mutex`?]

Thanks,

- Jules


On Sat, 17 Jun 2023 18:03:52 +0000
flightgear-commitlogs--- via Flightgear-commitlogs
<fli...@li...> wrote:

> amalon pushed a commit to branch next
> in repository simgear.
> 
> The following commit(s) were added to refs/heads/next by this push:
>      new 976b1372 props: Over-protect nodeOriginsLock
> 976b1372 is described below
> 
> SF URL:
> http://sourceforge.net/p/flightgear/simgear/ci/976b1372e631951752d391fc6fbd554611132eef/
> 
> Commit:    976b1372e631951752d391fc6fbd554611132eef
> Author:    James Hogan
> Committer: James Hogan
> AuthorDate:    Fri Jun 16 22:12:07 2023 +0100
> 
> props: Over-protect nodeOriginsLock
> 
> Over-protect the nodeOriginsLock against use after destruction, by
> flagging when the lock has been destructed and no longer using the
> lock after that point.
> 
> This should protect against SGPropertyNode objects being destroyed
> after nodeOriginsLock causing issues on some platforms such as
> possibly some Mac platforms.
> 
> 
> 
> ---
>  simgear/props/props.cxx | 42
> +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39
> insertions(+), 3 deletions(-)
> 
> diff --git a/simgear/props/props.cxx b/simgear/props/props.cxx
> index 54cf29f9..5e9cff79 100755
> --- a/simgear/props/props.cxx
> +++ b/simgear/props/props.cxx
> @@ -2170,8 +2170,44 @@ SGPropertyNode::clearValue ()
>   */
>  const int SGPropertyNode::LAST_USED_ATTRIBUTE = VALUE_CHANGED_DOWN;
>  
> +namespace {
> +/// A lock that is vaguely usable after destruction.
> +class NecroLock
> +{
> +public:
> +    ~NecroLock()
> +    {
> +        // Record that the lock should no longer be considered valid
> +        usable = false;
> +    }
> +
> +    /// Lock for reading if possible.
> +    std::shared_lock<std::shared_mutex> lockReading()
> +    {
> +        std::shared_lock<std::shared_mutex> ret;
> +        if (usable)
> +            ret = std::shared_lock(lock);
> +        return ret;
> +    }
> +
> +    /// Lock for writing if possible.
> +    std::unique_lock<std::shared_mutex> lockWriting()
> +    {
> +        std::unique_lock<std::shared_mutex> ret;
> +        if (usable)
> +            ret = std::unique_lock(lock);
> +        return ret;
> +    }
> +
> +protected:
> +    /// Allow for fallback to lockless after destruction.
> +    bool usable = true;
> +    /// The lock.
> +    std::shared_mutex lock;
> +};
> +} // namespace
>  /// Mutex to protect access to nodeOrigins.
> -static std::shared_mutex nodeOriginsLock;
> +static NecroLock nodeOriginsLock;
>  
>  typedef std::map<const SGPropertyNode*, SGSourceLocation>
> NodeOriginMap; /**
> @@ -2868,7 +2904,7 @@ SGPropertyNode::getPath (bool simplify) const
>  
>  SGSourceLocation SGPropertyNode::getLocation() const
>  {
> -    std::shared_lock rlock(nodeOriginsLock);
> +    auto rlock = nodeOriginsLock.lockReading();
>      if (!nodeOrigins)
>          return SGSourceLocation();
>      auto it = nodeOrigins->find(this);
> @@ -2880,7 +2916,7 @@ SGSourceLocation SGPropertyNode::getLocation()
> const 
>  void SGPropertyNode::setLocation(const SGSourceLocation& location)
>  {
> -    std::unique_lock lock(nodeOriginsLock);
> +    auto lock = nodeOriginsLock.lockWriting();
>      if (location.isValid()) {
>          if (!nodeOrigins)
>              nodeOrigins = new NodeOriginMap;
> 
> 
> _______________________________________________
> Flightgear-commitlogs mailing list
> Fli...@li...
> https://lists.sourceforge.net/lists/listinfo/flightgear-commitlogs



-- 
http://op59.net