Skip to content

Clang miscompiling code when using -fms-hotpatch and optimizations #76958

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.

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

Closed
MolecularMatters opened this issue Jan 4, 2024 · 2 comments
Closed

Comments

@MolecularMatters
Copy link

MolecularMatters commented Jan 4, 2024

When using the -fms-hotpatch option (which is a requirement for Live++), the tailcall/sibling call optimizations will lead to clang miscompiling function calls.

Example code:


extern "C" {
    void* mi_new(size_t size);
}

void *mi_new_test(size_t count)
{
    return mi_new(count);
}

void *builtin_malloc_test(size_t count)
{
    return __builtin_malloc(count);
}

Using -O1, the code will compile to the following:

"?mi_new_test@@YAPEAX_K@Z":             # @"?mi_new_test@@YAPEAX_K@Z"
        jmp     mi_new                          # TAILCALL
"?builtin_malloc_test@@YAPEAX_K@Z":     # @"?builtin_malloc_test@@YAPEAX_K@Z"
        jmp     malloc                          # TAILCALL

However, adding -fms-hotpatch, the code will miscompile to this:

"?mi_new_test@@YAPEAX_K@Z":             # @"?mi_new_test@@YAPEAX_K@Z"
        xchg    ax, ax

"?builtin_malloc_test@@YAPEAX_K@Z":     # @"?builtin_malloc_test@@YAPEAX_K@Z"
        xchg    ax, ax

As a workaround, specifying -fno-optimize-sibling-calls will produce the correct (but suboptimal) following code:

"?mi_new_test@@YAPEAX_K@Z":             # @"?mi_new_test@@YAPEAX_K@Z"
        sub     rsp, 40
        call    mi_new
        nop
        add     rsp, 40
        ret
"?builtin_malloc_test@@YAPEAX_K@Z":     # @"?builtin_malloc_test@@YAPEAX_K@Z"
        sub     rsp, 40
        call    malloc
        nop
        add     rsp, 40
        ret

Please refer to this godbolt compiler explorer repro: https://gcc.godbolt.org/z/r86nYbqvo

@github-actions github-actions bot added the clang Clang issues not falling into any other category label Jan 4, 2024
@sylvain-audi
Copy link
Contributor

Linking this issue with #59039, it looks like it's the same pattern.

@shafik
Copy link
Collaborator

shafik commented Jan 4, 2024

Duplicate of: #76879

@shafik shafik closed this as not planned Won't fix, can't repro, duplicate, stale Jan 4, 2024
@shafik shafik added the duplicate Resolved as duplicate label Jan 4, 2024
@EugeneZelenko EugeneZelenko added llvm:optimizations and removed clang Clang issues not falling into any other category labels Jan 5, 2024
aganea added a commit that referenced this issue Jan 22, 2024
…77245)

Previously, tail jump pseudo-opcodes were skipped by the
`encodeInstruction()` call inside `X86AsmPrinter::LowerPATCHABLE_OP`.
This caused emission of a 2-byte NOP and dropping of the tail jump.

With this PR, we change `PATCHABLE_OP` to not wrap the first
`MachineInstr` anymore, but inserting itself before,
leaving the instruction unaltered. At lowering time in `X86AsmPrinter`,
we now "look ahead" for the next non-pseudo `MachineInstr` and
lower+encode it, to inspect its size. If the size is below what
`PATCHABLE_OP` expects, it inserts NOPs; otherwise it does nothing. That
way, now the first `MachineInstr` is always lowered as usual even if
`"patchable-function"="prologue-short-redirect"` is used.

Fixes #76879,
#76958 and
#59039
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants