Fix bad logical-and implementation

[nosba0957]

The previous implementation of the logical-and operation produced incorrect result. This error also affected the funtionality of if statements using logical-and.

This implementation follows the C99 standard for logical-and, ensuring left-to-right evaluation. If the first operand is zero, the second operand is not evaluated.

Close #137

[jserv]

Can you add some test cases in tests/driver.sh to reflect the proposed changes?

[DrXiao]

Hi, @nosba0957 , I use the following code to test the logical-and operation:

/* test.c */
#include <stdio.h>

int func(int x)
{
    if (x >= 0) {
        printf("x >= 0\n");
    }   
    return x;
}   

int main()
{
    int ret = 0;
    
    ret = 1 && func(5);
    if (ret)                   
        printf("%d\n", ret);

    printf("============\n");
        
    ret = 0 && func(5);
    if (ret)
        printf("%d\n", ret);

    return 0;
}

If we use GCC or Clang to compile and execute the code, both yield the same output:

$ gcc -o test-gcc test.c && ./test-gcc
x >= 0
1
============

$ clang -o test-clang test.c && ./test-clang
x >= 0
1
============

In the main function, for the logical-and operation, because the first operand of the second if statement is zero, its second operand, which calls the function func(), will not be evaluated. (This follows the left-to-right evaluation rule.)

However, if we use shecc with the modified implementation to generate an executable with Arm32, the result is unexpected:

$ qemu-arm out/shecc-stage2.elf -o test-shecc-stage2 test.c && qemu-arm test-shecc-stage2
x >= 0
5
============
x >= 0

For the second if statement, although the first operand is zero, the second operand (func(5)) is still executed. Besides, the result of the logical-and should be 0 or 1, but the proposed changes may cause the second operand's value being returned.

Therefore, I think we can ensure the correctness of the logical-and behavior's result, but we still need to have more effort to implement the left-to-right evaluation rule for logical-and and logical-or operations.

[nosba0957]

Sure, I'll add test cases to tests/driver.sh.

@DrXiao, Thank you for your feedback. I've decided to first ensure the correctness of the logical-and operation. Currently, left-to-right evaluation cannot be achieved. I will revise the commit messages.

[jserv]

Currently, left-to-right evaluation cannot be achieved.

Per the C99 standard, left-to-right evaluation should be guaranteed. You should investigate the root cause preventing the proposed changes from being compliant with the C standard.

[nosba0957]

@vacantron, I'm currently modifying the parser to achieve left-to-right evaluation. But if I start modifying the implementation from the parser, the changes to the codegen might not be necessary.

[vacantron]

@vacantron, I'm currently modifying the parser to achieve left-to-right evaluation. But if I start modifying the implementation from the parser, the changes to the codegen might not be necessary.

That's okay. You can close this pull request since then.

[jserv]

Use git rebase instead of git merge.

[nosba0957]

In src/ssa.c, within the insert_phi_insn function, why we didn't link the prev pointer between the original instruction and the new phi-func?
This can cause errors in append_unwound_phi_insn. For example, if a basic block has two instructions:

phi %a
branch %b

When we execute append_unwound_phi_insn, some phi-functions need to insert new unwind-phi instruction in this basic block. If a branch instruction is detected at the end of basic block, it will insert unwind-phi instruction before the branch. However, we do not link prev from branch %b to phi %a, so phi %a will be directly replaced when inserting the unwind-phi instruction.
When attempting to link the prev pointer in insert_phi_insn, it ends with segmentation fault. Therefore, I attempted to modify append_unwound_phi_insn to search for a place for unwind_phi instruction from head of insn_list. It finally runs correctly.

This problem can be reproduced by removing the modification in append_unwound_phi_insn and executing a simple example of do-while loop with logical-and condition expression.

[vacantron]

In src/ssa.c, within the insert_phi_insn function, why we didn't link the prev pointer between the original instruction and the new phi-func?

The property prev was added later and I forgot to update the related code. Thank you for pointing it out.

[vacantron]

There is a snapshot test under the tests/ directory to watch the consistence of the work of lever/parser. Since they have been changed, the corresponding snapshots need to be updated together.

You can run tests/update-snapshots.sh under the ARM config.

[nosba0957]

The property prev was added later and I forgot to update the related code. Thank you to point it out

So should I keep the modifications in append_unwound_phi_insn for now?

You can run tests/update-snapshots.sh under the ARM config.

Thanks for your friendly reminder.

[vacantron]

So should I keep the modifications in append_unwound_phi_insn for now?

You can add a comment like FIXME: ... to indicate the workaround.

[jserv]

Can you avoid traversing the whole elements since we just want to figure out the branch operation at first glance?

[vacantron]

@nosba0957 , I have found the reason of this error. I will open another pull request for it later.

[nosba0957]

Should I still modify this part of the code, or should I keep the current commit as is and wait for @vacantron 's fix?

[vacantron]

You can cherry-pick the commit at vacantron@5fd2fbe and update your change.

[vacantron]

@nosba0957,I have opened the #137. You can redirect this pull request to that issue.

[jserv]

Rebase the latest master branch.

[jserv]

Thank @nosba0957 for contributing!