[DWARFLinkerParallel] Change more cases of compare_exchange_weak to compare_exchange_strong

[mstorsjo]

This is a follow-up to 07bc54b; this seems to perhaps fix occasional crashes in dsymutil on Windows on aarch64.

While these are used in loops, which could be tolerant to concurrent accesses, the overall logic seems to assume that they don't have spurious failures; I've observed one case in a debugger, where LastGroup == nullptr within the loop in ArrayList::add().

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

[mstorsjo]

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

Can you have a look at these cases? (I haven't run into issues with them, but the code looks like it could lock up.)

[avl-llvm]

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

Can you have a look at these cases? (I haven't run into issues with them, but the code looks like it could lock up.)

yes, you are right those places look incorrect. Thank you for pointing to this.

Instead of

      auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement))) {
      }

there should be something like

      do {
        auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement)));

and loop should be added to:

      auto InputData = Flags.load();
      if ((InputData & 0x7) == NotSet)
        if (Flags.compare_exchange_weak(InputData, (InputData | Placement)))
          return true;

Would you prefer me to do these changes or would you prefer to do these changes youself?

[mstorsjo]

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

Can you have a look at these cases? (I haven't run into issues with them, but the code looks like it could lock up.)

yes, you are right those places look incorrect. Thank you for pointing to this.

Instead of

      auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement))) {
      }

there should be something like

      do {
        auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement)));

and loop should be added to:

      auto InputData = Flags.load();
      if ((InputData & 0x7) == NotSet)
        if (Flags.compare_exchange_weak(InputData, (InputData | Placement)))
          return true;

Would you prefer me to do these changes or would you prefer to do these changes youself?

Yes, that looks correct to me. As you're more familiar with the overall intent here (I'm just trying to debug things that show up when running ninja check ;-) ) I'd prefer if you'd open such a PR, but I can review it!

[avl-llvm]

Yes, that looks correct to me. As you're more familiar with the overall intent here (I'm just trying to debug things that show up when running ninja check ;-) ) I'd prefer if you'd open such a PR, but I can review it!

Ok, Will do that way(open PR and do changes) Thank you again for finding this!

[avl-llvm]

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

Can you have a look at these cases? (I haven't run into issues with them, but the code looks like it could lock up.)

yes, you are right those places look incorrect. Thank you for pointing to this.
Instead of

      auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement))) {
      }

there should be something like

      do {
        auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement)));

Would you prefer me to do these changes or would you prefer to do these changes youself?

Yes, that looks correct to me. As you're more familiar with the overall intent here (I'm just trying to debug things that show up when running ninja check ;-) ) I'd prefer if you'd open such a PR, but I can review it!

My answer was too hurry. This code looks OK:

auto InputData = Flags.load();
while (!Flags.compare_exchange_weak(InputData,
                                    ((InputData & ~0x7) | Placement))) {
}

There should not be endless loop here. if there would be a true concurrent write, leading to compare_exchange_weak() returning false, then InputData would be updated with current value of "Flags". Then it would - either match on next iteration and loop will finish, either(in case new concurrent write) it would be updated again.

So, after this MR was integrated, it looks like no more other issues inside dwarflinker parallel with usages of compare_exchange_weak.

[mstorsjo]

The remaining uses of compare_exchange_weak() in
DWARFLinkerCompileUnit.h also seem very suspicious; if there would be a true concurrent write, leading to compare_exchange_weak() returning false, it seems like we would get stuck in an endless loop - but changing them to compare_exchange_strong() wouldn't make any difference with respect to that.

Can you have a look at these cases? (I haven't run into issues with them, but the code looks like it could lock up.)

yes, you are right those places look incorrect. Thank you for pointing to this.
Instead of

      auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement))) {
      }

there should be something like

      do {
        auto InputData = Flags.load();
      while (!Flags.compare_exchange_weak(InputData,
                                          ((InputData & ~0x7) | Placement)));

Would you prefer me to do these changes or would you prefer to do these changes youself?

Yes, that looks correct to me. As you're more familiar with the overall intent here (I'm just trying to debug things that show up when running ninja check ;-) ) I'd prefer if you'd open such a PR, but I can review it!

My answer was too hurry. This code looks OK:

auto InputData = Flags.load();
while (!Flags.compare_exchange_weak(InputData,
                                    ((InputData & ~0x7) | Placement))) {
}

There should not be endless loop here. if there would be a true concurrent write, leading to compare_exchange_weak() returning false, then InputData would be updated with current value of "Flags". Then it would - either match on next iteration and loop will finish, either(in case new concurrent write) it would be updated again.

So, after this MR was integrated, it looks like no more other issues inside dwarflinker parallel with usages of compare_exchange_weak.

Oh, indeed - you're right, I forgot that it updates the InputData variable with the new value on change. Yes then those things should indeed be ok.