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

[Michael Dardis]

Thanks,

I will try both of these today.

Michael

On 12 Nov. 2017 16:44, "David Holmes" <david.holmes at oracle.com> 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
>
>