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

[David Holmes]

I've filed:

https://bugs.openjdk.java.net/browse/JDK-8191102

for the include issue.

But I don't think this is caused by two-phase-name-lookup as that is not 
yet enabled by default in VS2017.

David

On 12/11/2017 3:44 PM, David Holmes wrote:
> 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
>