Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8227369

pd_disjoint_words_atomic() needs to be atomic

    Details

    • Subcomponent:
    • CPU:
      generic
    • OS:
      generic

      Description

      This issue came up during the code review for the following fix:

           JDK-8227338 templateInterpreter.cpp: copy_table() needs to be safer
           https://bugs.openjdk.java.net/browse/JDK-8227338

      On 7/6/19 6:06 PM, David Holmes wrote:
      > Hi Dan,
      >
      >> http://cr.openjdk.java.net/~dcubed/8227338-webrev/0_for_jdk14/
      >
      > So the original code uses a loop to copy, while the new code calls
      > Copy::disjoint_words_atomic, but the implementation of that on x64
      > is just a loop same as the original AFAICS:
      >
      > static void pd_disjoint_words_atomic(const HeapWord* from, HeapWord* to, size_t count) {
      > #ifdef AMD64
      > switch (count) {
      > case 8: to[7] = from[7];
      > case 7: to[6] = from[6];
      > case 6: to[5] = from[5];
      > case 5: to[4] = from[4];
      > case 4: to[3] = from[3];
      > case 3: to[2] = from[2];
      > case 2: to[1] = from[1];
      > case 1: to[0] = from[0];
      > case 0: break;
      > default:
      > while (count-- > 0) {
      > *to++ = *from++;
      > }
      > break;
      > }
      > #else
      >
      > David
      > -----

      Erik replied with:

      On 7/7/19 4:48 AM, Erik Osterlund wrote:
      > Yeah that switch statement code and yet another plain non-volatile load/store loop looks like complete nonsense unfortunately. It should at least use Atomic::load/store.
      >
      > Fortunately, on x86_64, I believe it will in practice yield word atomic copying anyway by chance. But it should be fixed anyway. *sigh*
      >
      > The real danger is SPARC though and its BIS instructions. I don't have the code in front of me, but I really hope not to see that switch statement and non-volatile loop in that pd_disjoint_words_atomic() function.
      >
      > And I agree that the atomic copying API should be used when we need atomic copying. And if it turns out the implementation of that API is not atomic, it should be fixed in that atomic copying API.
      >
      > So I think this change looks good. But it looks like we are not done yet. :c
      >
      > Thanks,
      > /Erik

      So we need to take a closer look at the various implementations
      of pd_disjoint_words_atomic() to make sure they are intentionally
      atomic and not accidentally atomic.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                Unassigned
                Reporter:
                dcubed Daniel Daugherty
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated: