Clang generates wrong code on armeabi-v7a for string literal pointers (over-optimization)

[achurch]

Clang from NDK r16b targeting armeabi-v7a can over-optimize loads of string literal pointers such that the value of a const char * variable to which a string literal has been assigned changes depending on where the variable is referenced. Specifically, if the string is stored in a local constant block in the code stream and the second reference to the variable occurs past the point where that string can be referenced by an add/sub PC instruction, Clang may put a second copy of the string in a subsequent local constant block and load the variable's value by referencing that copy, resulting in a different pointer value than was originally stored in the variable.

This is a regression from r13b (r14b and r15c are likewise broken; I haven't tested any other intermediate releases).

Test case: https://gist.github.com/achurch/3bca2069a0f25db98df764b6711dc83d

I haven't been able to minimize the test case as much as I'd like (in trivial code, the compiler seems to reload from the stack rather than loading a literal pointer with add/sub PC), but the bug occurs in TEST_UTF8("\xF9\x80\x80\x80\x80", 0x1000000, 5) because the if (_s != _string + _consume) test in the macro loads the value of _string from a different string literal than the initial assignment of _string to _s:

 4c8:   e28d000c        add     r0, sp, #12
 4cc:   e24f4ff3        sub     r4, pc, #972    ;references offset 0x108
 4d0:   e58d400c        str     r4, [sp, #12]
 4d4:   ebfffffe        bl      0 <utf8_read>
                        4d4: R_ARM_CALL utf8_read
(...)
 564:   e59d000c        ldr     r0, [sp, #12]
 568:   e28f1ff3        add     r1, pc, #972    ;references offset 0x93c (wrong!)
 56c:   e2811005        add     r1, r1, #5
 570:   e1500001        cmp     r0, r1

Note that there is only one reference to the string literal within the macro; it assigns the string literal to a local variable precisely to avoid the possibility of two identical string literals having different addresses.

[rprichard]

Reduced test case:

/*
/ssd/stand-arm-14-r16b/bin/clang -pie -fPIE literal.c -o literal -O2 && \
    adb push literal /data/local/tmp && \
    adb shell /data/local/tmp/literal
*/

int puts(const char *msg);

volatile int dummy = 0;
volatile int expected = 42;
volatile int failed = 0;

__attribute__((weak)) const char *identity(const char *val) {
    return val;
}

__attribute__((weak)) void fail(const char *msg) {
    puts(msg);
    failed = 1;
}

#define X() do { dummy++; dummy++; dummy++; dummy++; dummy++; dummy++; } while (0)

int main(void) {
    const char *const literal = "1";
    const char *s = literal;
    s = identity(s);
    X(); X(); X(); X(); X(); X(); X(); X(); X(); X(); X();
    X(); X(); X(); X(); X(); X(); X(); X(); X(); X(); X();
    X(); X(); X(); X(); X(); X(); X(); X(); X(); X(); X();
    if (expected != 42) {
        fail("FAILURE-1");
    }
    if (s != literal) {
        fail("FAILURE-2");
    }
    return failed;
}

Output:

FAILURE-2

[rprichard]

This issue has already been filed upstream as https://bugs.llvm.org/show_bug.cgi?id=32780. The upstream workaround was to turn off the constant pool promotion, which seems to be a size optimization:

• https://reviews.llvm.org/rL282387
• https://reviews.llvm.org/D33446

The compiler in the upcoming NDK r17 already leaves off the optimization off by default.

To turn it off with the Clang in older NDKs (at least with r15c and up), add -mllvm -arm-promote-constant=0 to the compiler flags.

I reproduced this problem with r15c and r16b but not with r13b or r14b. The Clang in r13b/r14b also didn't recognize the -arm-promote-constant option.

[rprichard]

@stephenhines You might be interested in this.

I don't think there's anything more to do here, unless we want to try fixing the optimization upstream. It looks like someone tried fixing it about 9 months ago. (https://bugs.llvm.org/show_bug.cgi?id=32780#c12)

I wonder how much of a size regression it's going to cause from r16 -> r17. The r17 compiler disables the constpool promotion optimization, which tries to embed small constant variables into .text rather than .rodata.

[stephenhines]

I commented on the original bug and am following up with ARM about fixing/re-enabling that optimization. Thanks for tracking down the issue and finding the original bug.

[achurch]

Thanks from me as well for finding the original bug.

Just to follow up on the r14b aspect, I tried again and was not able to reproduce with r14b, so I probably just ran the wrong binary for that test. Sorry about that.