RFR (M): 8188224: Generalize Atomic::load/store to use templates

[David Holmes]

Hi Kim,

Many thanks for taking a look.

On 12/11/2017 2:47 PM, Kim Barrett wrote:
>> On Nov 10, 2017, at 10:30 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Mikael,
>>
>> On 11/11/2017 10:59 AM, Michael Dardis wrote:
>>> Hi David,
>>> Unfortunately I'm using VS2017 for a couple of reasons (64bit compiler in community edition, improved UWP support). Everything else works fine with a couple of very minor tweaks, but I'm having real trouble trying to deal with the templates here and figure out what's missing.
>>
>> I've cc'd our C++ guru Kim Barrett.
> 
> I saw this, but haven’t had a chance to seriously look at it yet.
> 
> One thing I did notice, though, is that all the OrderAccess definitions (as opposed to declarations) are in
> orderAccess.inline.hpp.  That doesn’t appear to be a change from the pre-templatization; all the pre-template
> definitions were in the inline.hpp file too.
> 
> However, the offending call to OrderAccess::release_store is in a .hpp file (classLoader.hpp), which only
> includes orderAccess.hpp (and it shouldn’t include orderAccess.inline.hpp, because .hpp files are not
> supposed to include .inline.hpp files).  Which means the call in classLoader.hpp might only see the
> declaration, and not the definition.  That seems like a bug.

Yes that needs to be fixed. Though exact fix seems a little tricky if we 
still want the referring function to be inlined.

> I haven’t thought this through carefully, but this might be a two-phase name lookup issue.  Note that
> VS2017 is the first version of VS that has two-phase name lookup.  Other compilers (gcc & clang)
> have had two-phase lookup for a (very!) long time.  Assuming this is the right area to be looking (and
> I’m not sure of that yet), it could be that VS2017 is being more strict than those other compilers (that
> aren’t complaining about this code but maybe should?), or that VS2017 has a bug in this new (to it)
> area.
> 
> There is a pretty thorough discussion of the two-phase name lookup issue in VS here:
> https://blogs.msdn.microsoft.com/vcblog/2017/09/11/two-phase-name-lookup-support-comes-to-msvc/
> It also discusses how to obtain the old behavior, using /Zc:twoPhase-.  It would be interesting to know
> whether that made any difference for this problem.  (I don’t have easy access to VS2017 or even have
> any familiarity with using it, or Windows for that matter.)

Okay - so we may have a workaround pending the fix. Hopefully Michael 
can test this out.

Thanks again.
David