translation of upc_forall missing braces required for correct code

[PHHargrove]

I've just tried for the first time using the clang built with clang-upc2c as the backend compiler. There are many new "expect pass but got warning" failures in a harness run, and more investigation of those is needed (most are probably due to the tests themselves). HOWEVER, one at least is the result of the translator.

Given

  upc_forall(int i=0; i<N; i++; i)
    if (A[i] != MYTHREAD) errflag=1;

the .trans.c contains the following translation of the upc_forall:

        if (upcrt_forall_control)
            for (int i = 0; i < N; i++)
                if ((upcr_get_pshared(&_bupc_spilld0, upcr_add_pshared1(A, 4U, i), 0U, 4U) , _bupc_spilld0) != (int)upcr_mythread())
                    errflag = 1;
        else {
            upcrt_forall_control = 1;
            for (int i = 0; i < N; i++)
                if ((i) % upcr_threads() == upcr_mythread())
                    if ((upcr_get_pshared(&_bupc_spilld0, upcr_add_pshared1(A, 4U, i), 0U, 4U) , _bupc_spilld0) != (int)upcr_mythread())
                        errflag = 1;
            upcrt_forall_control = 0;
        }

That is entirely valid C code, but notice that the line immediately before the translator-supplied instance of else in that code is the if from the user's code. This results in the backend clang warning:

foo.trans.c:18:9: warning: add explicit braces to avoid dangling else [-Wdangling-else]
        else {
        ^
1 warning generated.

If necessary, I am OK with passing Wno-dangling-else to clang as a backend if that is necessary. However, I wanted to first report the issue here to see if it is reasonable to instead ask/expect/require that clang-upc2c generate output that clang itself is willing to accept as input. Thoughts?

[nenadv]

clang-upc2c should produce the code that would not cause a warning.

On 3/7/14, 5:19 PM, Paul H. Hargrove wrote:

I've just tried for the first time using the clang built with clang-upc2c as the backend compiler. There are /many/ new "expect pass but got warning" failures in a harness run, and more investigation of those is needed (most are probably due to the tests themselves). HOWEVER, one at least is the result of the translator.

Given

upc_forall(int i=0; i<N; i++; i) if (A[i] != MYTHREAD) errflag=1;

the .trans.c contains the following translation of the |upc_forall|:

if (upcrt_forall_control) for (int i = 0; i < N; i++) if ((upcr_get_pshared(&_bupc_spilld0, upcr_add_pshared1(A, 4U, i), 0U, 4U) , _bupc_spilld0) != (int)upcr_mythread()) errflag = 1; else { upcrt_forall_control = 1; for (int i = 0; i < N; i++) if ((i) % upcr_threads() == upcr_mythread()) if ((upcr_get_pshared(&_bupc_spilld0, upcr_add_pshared1(A, 4U, i), 0U, 4U) , _bupc_spilld0) != (int)upcr_mythread()) errflag = 1; upcrt_forall_control = 0; }

That is /entirely/ valid C code, but notice that the line immediately before the translator-supplied instance of |else| in that code is the |if| from the user's code. This results in the /backend/ clang warning:

foo.trans.c:18:9: warning: add explicit braces to avoid dangling else [-Wdangling-else] else { ^ 1 warning generated.

If necessary, I am OK with passing |Wno-dangling-else| to clang as a backend if that is necessary. However, I wanted to first report the issue here to see if it is reasonable to instead ask/expect/require that clang-upc2c generate output that clang itself is willing to accept as input. Thoughts?

— Reply to this email directly or view it on GitHub https://github.com/Intrepid/clang-upc/issues/44.

[swatanabe]

On 03/07/2014 05:19 PM, Paul H. Hargrove wrote:

That is entirely valid C code, but notice that the line immediately before the translator-supplied instance of else in that code is the if from the user's code. This results in the backend clang warning:

Just because it's valid doesn't mean that it does what you want. In this case, the warning is correct. The translation needs to add braces.

[PHHargrove]

In this case, the warning is correct.

Ah, right. So the .trans.c code is actually wrong!

The translation needs to add braces.

Is this likely to be a matter of the translation or a problem with StmtPrinter thinking it can omit the braces when the body was a single statement?

[swatanabe]

On 03/07/2014 06:14 PM, Paul H. Hargrove wrote:

The translation needs to add braces.

Is this likely to be a matter of the translation or a problem with StmtPrinter thinking it can omit the braces when the body was a single statement?

It's a problem with the translation. StmtPrinter never adds braces. The CompoundStmt needs to be represented in the AST.

[PHHargrove]

It's a problem with the translation. StmtPrinter never adds braces.

Well, then I guess I reported the issue against the wrong repo. ;)

[PHHargrove]

Fixed in https://github.com/Intrepid/upc2c/commit/93b2422f845b8572ee321ea7814a6c887cc88135