XrdSys: Undefined behavior in the poller

[bbockelm]

Here, the wakePend variable is written to without a lock:

https://github.com/xrootd/xrootd/blob/master/src/XrdSys/XrdSysIOEventsPollE.icc#L210

while here, it's read with a lock:

https://github.com/xrootd/xrootd/blob/master/src/XrdSys/XrdSysIOEvents.cc#L1162

This causes undefined behavior according to the C++11 standard and is illegal.

[abh3]

OK, I have an issue with this slavish adherence to C+11 undefined behavior talk. Ya know, sometimes undefined behavior is just perfectly OK as it is in this particular case. This is an optimization flag that does nothing more than try to reduce the number of duplicate messages that are being sent. It's perfectly OK to send them we just want to reduce them. The place where the memory location is set without a lock is high traffic and we don't want a lock, we don't even want an atomic, and we don't care if we get undefined behavior. That is not the point. We are trying to save compute cycles in a high use path and optimize a bit in a lower use path. So, I'm not pulling this in.

[bbockelm]

Hi Andy,

Did you try to look at the patch? As explained in the pull request and the ticket, the code generated on x86 is identical -- in fact, the assembler generated should be the same on all platforms except DEC Alpha.

As policy, CMS treats undefined behavior - especially race conditions - as serious code errors. We're going to have to carry this patch, meaning there is a higher "activation energy" for CMS to test and include new Xrootd features (that is, I'll need to spend more time maintaining the CMS-specific tree and less time on other items).

Brian

[abh3]

Yes, I did look at the patch. I certainly believe that memory_order_relaxed generates the same code and I suppose I sort of believe that consume/release would generate the same code on practically all architectures if you have faith in the C++11 reference that tells you that is the case. Then again, there has been at least one instance where a compiler didn't generate the same code to the dismay of a lot of people. Also, consume/release do impact optimization around the operation so the final results will likely vary over time. Let me think about this.

[abh3]

OK, I convinced myself that this will be OK but could you do me a big favor and consolidate the use of load() and store() into SysAtomics under control of USE_CPP11_ATOMICS? I propose the following

CPP_LOAD(var) becomes either var.load(std::memory_order_consume) or just var

CPP_STORE(var, val) becomes var.store(val, std::memory_order_release) or var = val

CPP_MUTEX(stmt) becomes stmt or doesn't inline stmt

This means moving the code now in XrdSys/XrdSysPthread.hh to XrdSys/XrdSysAtomics.hh
I say this because if/def sprinkling makes the code hard to read and I know there are other instances of this in the code (e.g. pre-lock tests). That way we will be in the position of easily making the modification.

[bbockelm]

Sure, I'd be happy to do this. How about something like:

CPP11_ATOMIC_LOAD(var, std::memory_order_consume)
-> var, OR
-> var.load(std::memory_order_consume)

This gives a bit of wiggle room for different memory orderings. Further, we'd need:

CPP11_ATOMIC_TYPE(bool)
-> bool, OR
-> std::atomic

This way, we can define:

CPP11_ATOMIC_TYPE(bool) wakePend;

[ljanyst]

I'd drop the CPP11_ prefix?

[bbockelm]

Yeah - I struggled with that one a bit. A few things that I wanted to communicate with that:

• This is different from the rest of the atomics (all the other calls are strongly consistent and can be much more expensive).
• Behavior, when enabled, aligns with C++11.
• Without C++11 enabled, these are not atomics at all!

[abh3]

That's why I went with CPP and just dropped the 11. This is clearly a compiler thing and it does whatever it has to do for the platform. Yes, it's introduced in C++11 but we know that is going to be carried forward to C++14 and future specs. It's also likely to be unchanged, other than additional options. So, CPP seemed like the right thing to do and leave off any reference to a specific compiler.

[bbockelm]

Ok, sounds good to me! Expect an update in a day or so.

[ljanyst]

Closing as the discussion moved to the pull request #170.