Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Error "Unsequenced modification and access" when building distribution from sources #1487

Closed
gianlucabertani opened this issue Oct 2, 2020 · 7 comments
Closed

Error "Unsequenced modification and access" when building distribution from sources #1487

gianlucabertani opened this issue Oct 2, 2020 · 7 comments

Comments

@gianlucabertani
Copy link
Contributor

Hello,

I'm trying to build the distribution from scratch from release 2.7 and Xcode 12.0.1, with the only change to default build parameters being the full list of supported architectures:

$ export J2OBJC_ARCHS="iphone simulator macosx iphone64 iphone64e watchv7k watch64 watchsimulator simulator64 appletvos appletvsimulator maccatalyst"
$ make dist

When it gets to the compilation of Striped64.m I get the following error:

compiling /Users/gbertani/j2objc-2.7/jre_emul/build_result/Classes/java/util/concurrent/atomic/Striped64.m
/Users/gbertani/j2objc-2.7/jre_emul/build_result/Classes/java/util/concurrent/atomic/Striped64.m:132:91: error:
      unsequenced modification and access to 'v' [-Werror,-Wunsequenced]
  ...v = JreLoadVolatileLong(&a->value_), (fn == nil) ? v + x : [((id<JavaUtilFunctionLongBinaryOperator>) nil_chk(fn...
       ^                                                ~
/Users/gbertani/j2objc-2.7/jre_emul/build_result/Classes/java/util/concurrent/atomic/Striped64.m:160:82: error:
      unsequenced modification and access to 'v' [-Werror,-Wunsequenced]
  ...v = JreLoadVolatileLong(&base_), (fn == nil) ? v + x : [((id<JavaUtilFunctionLongBinaryOperator>) nil_chk(fn)) a...
       ^                                            ~
2 errors generated.
make[1]: *** [/Users/gbertani/j2objc-2.7/jre_emul/build_result/objs-iphone/java/util/concurrent/atomic/Striped64.o] Error 1
make: *** [jre_emul_dist] Error 2

I have read both issues #338 and #343, but none of the suggestions worked:

  • First I tried fixing the expression as (v + x), but the error persisted.
  • Then I tried adding --extract-unsequenced to TRANSLATE_ARGS in translate.mk, then cleaning and rebuilding, but the error also persisted.

Any other suggestion?
@tomball
Copy link
Collaborator

tomball commented Oct 2, 2020

One of the joys of being an Xcode developer is that with each release there's the chances that new clang are added to what it considers the default set. For Xcode 12, this adds -Wunsequenced. While I generally support enabling all useful warnings, it's still a hassle when they arrive unannounced.

To fix any new warning that is breaking the build, take its name and prepend it with "no-". So in this case, the failing warning is "-Wunsequenced", and the workaround is to specify "-Wno-unsequenced". The j2objc build has a WARNINGS build variable, so to suppress this warning, build using:

    $ make -j8 dist WARNINGS=-Wno-unsequenced

The fix probably needs to be in the translator, as it looks to be literally translating an equivalent Java expression.

The issue (as I currently understand it) is that Java guarantees that parameter expressions are executed left-to-right (JLS 15.12.4.2, but function parameters are not evaluated in a defined order in C/C++/ObjC. I think the right fix is for the translator to pull out the local variable assignment (v = a.value), and move it into block with the rest of the expression. That screws up the "else if" chain, though, so the rest of the chain also needs to move into the block (original Java, but this is a translator change):

             wasUncontended = true;      // Continue after rehash
-        else if (a.cas(v = a.value,
-                       (fn == null) ? v + x : fn.applyAsLong(v, x)))
-            break;
-        else if (n >= NCPU || cells != as)
-            collide = false;            // At max size or stale
-        else if (!collide)
-            collide = true;
-        else if (cellsBusy == 0 && casCellsBusy()) {
-            try {
-                if (cells == as)        // Expand table unless stale
-                    cells = Arrays.copyOf(as, n << 1);
-            } finally {
-                cellsBusy = 0;
-            collide = false;
-            continue;                   // Retry with expanded table
-        }
+        else {
+            v = a.value;
+            if (a.cas(v,
+                           (fn == null) ? v + x : fn.applyAsLong(v, x)))
+                break;
+            else if (n >= NCPU || cells != as)
+                collide = false;            // At max size or stale
+            else if (!collide)
+                collide = true;
+            else if (cellsBusy == 0 && casCellsBusy()) {
+                try {
+                    if (cells == as)        // Expand table unless stale
+                        cells = Arrays.copyOf(as, n << 1);
+                } finally {
+                    cellsBusy = 0;
+                }
+                collide = false;
+                continue;                   // Retry with expanded table
+             }
+         }
        h = advanceProbe(h);

Do you see a simpler change? Replacing the vs with a.value won't work, since it's possible for a.value to be changed by a different thread:

             wasUncontended = true;      // Continue after rehash
-        else if (a.cas(v = a.value,
-                       (fn == null) ? v + x : fn.applyAsLong(v, x)))
+        else if (a.cas(a.value,
+                       (fn == null) ? a.value + x : fn.applyAsLong(a.value, x))) // a.value may have 3 different values
             break;

@tomball
Copy link
Collaborator

tomball commented Oct 2, 2020

Another possibility is to extract the whole boolean expression into a private boolean method:

             wasUncontended = true;      // Continue after rehash
-        else if (a.cas(v = a.value,
-                       (fn == null) ? v + x : fn.applyAsLong(v, x)))
+        else if (__f(a, fn, x))
             break;
...
+    private boolean __f(Cell a, LongBinaryOperator fn, long x) {}
+        long v = a.value;
+        return a.cas(v, (fn == null) ? v + x : fn.applyAsLong(v, x));
+    }

Hmm, that's starting to look like the same extraction done for capturing lambdas. Have to think about this some more...

@tomball
Copy link
Collaborator

tomball commented Oct 2, 2020

Much simpler: it's a bug (uncaught edge case) in UnsequencedExpressionRewriter.

j2objc-copybara pushed a commit that referenced this issue Oct 3, 2020
@tomball
Issue #1487: fixes unsequenced expression warning when building with …
7be49c0 
…Xcode 12.

PiperOrigin-RevId: 335133260
@j2objc-copybara j2objc-copybara mentioned this issue Oct 3, 2020
Merged
@tomball
Copy link
Collaborator

tomball commented Oct 3, 2020

The workaround to build with -Wno-unsequenced will enable your build until this is fixed.

FYI, pull request #1488 currently only fixes the pattern found in Striped64.java. With it, the build now fails on another hot mess (err, I mean complex expression 😉) in android/util/Base64.java:

    v = (((tailLen > 1 ? tail[t++] : input[p++]) & 0xff) << 10) |
        (((tailLen > 0 ? tail[t++] : input[p++]) & 0xff) << 2);

I think what happened in Xcode 12 is that clang's unsequenced warning support was significantly improved. So this is a good thing in the long run, as we need to improve the translator to match their error detection.

j2objc-copybara pushed a commit that referenced this issue Oct 4, 2020
@tomball
Issue #1487: fixes unsequenced expression warning when building with …
a523b28 
…Xcode 12.

PiperOrigin-RevId: 335133260
@tomball
Copy link
Collaborator

tomball commented Oct 4, 2020

#1488 has been updated so the project now fully builds with Xcode 12.

@gianlucabertani
Copy link
Contributor Author

Thank you very much for your prompt support!

j2objc-copybara pushed a commit that referenced this issue Oct 5, 2020
@tomball
Issue #1487: fixes unsequenced expression warnings when building with…
6791a77 
… Xcode 12.

PiperOrigin-RevId: 335133260
j2objc-copybara pushed a commit that referenced this issue Oct 5, 2020
@tomball
Issue #1487: fixes unsequenced expression warnings when building with…
bc9aa77 
… Xcode 12.

PiperOrigin-RevId: 335509466
j2objc-copybara pushed a commit that referenced this issue Oct 5, 2020
@tomball
Issue #1487: fixes unsequenced expression warnings when building with…
4a67106 
… Xcode 12.

PiperOrigin-RevId: 335509466
@tomball
Copy link
Collaborator

tomball commented Oct 5, 2020

Fixed in #1488.

guillaumeaudet pushed a commit to mirego/j2objc that referenced this issue Nov 12, 2020
@tomball
Issue google#1487: fixes unsequenced expression warnings when buildin…
3b313c3 
…g with Xcode 12.

PiperOrigin-RevId: 335509466
guillaumeaudet pushed a commit to mirego/j2objc that referenced this issue Feb 4, 2021
@tomball
Issue google#1487: fixes unsequenced expression warnings when buildin…
6f56c72 
…g with Xcode 12.

PiperOrigin-RevId: 335509466
lukhnos pushed a commit to lukhnos/j2objc that referenced this issue May 7, 2021
@tomball @lukhnos
Issue google#1487: fixes unsequenced expression warnings when buildin…
9abefbf 
…g with Xcode 12.

PiperOrigin-RevId: 335509466
flambert pushed a commit to mirego/j2objc that referenced this issue Feb 1, 2023
@tomball @flambert
Issue google#1487: fixes unsequenced expression warnings when buildin…
ed155f8 
…g with Xcode 12.

PiperOrigin-RevId: 335509466
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tomball @gianlucabertani