Page MenuHomePhabricator

Error if inline assembly ends in unexpected mode
Needs ReviewPublic

Authored by niravd on May 9 2016, 7:19 AM.

Details

Reviewers
dwmw2
Summary

[x86] Inline assembly may issue directives that change the
bit-mode (i.e. .code16/.code32/.code64) but these changes do not do not
persist past the inline assembly call as is reasonable. In the case that
inline assembly terminates with a different mode, incorrect assembly may
be generated.

As this situation is likely a mistake and should be dealt with by the
user throw an error message.

Diff Detail

Event Timeline

niravd updated this revision to Diff 56572.May 9 2016, 7:19 AM
niravd retitled this revision from to Error if inline assembly ends in unexpected mode.
niravd added a reviewer: dwmw2.
niravd updated this object.
niravd added a subscriber: llvm-commits.
niravd updated this object.May 9 2016, 11:44 AM

ARM has a similar situation arise in their code but uses a different fix. There, this situation is fixed if needed if we use the integrated assembler and otherwise always emits a directive in no integrated assembler case. This guarantees any code llvm generates is always correct, but leaves no notification that there is a potential latent bug should you compile with a separate compiler.

We should be consistent here and either change ARM to also emit an error or change both to emit warnings.

rovka added a subscriber: rovka.May 11 2016, 4:45 AM

ARM has a similar situation arise in their code but uses a different fix. There, this situation is fixed if needed if we use the integrated assembler and otherwise always emits a directive in no integrated assembler case. This guarantees any code llvm generates is always correct, but leaves no notification that there is a potential latent bug should you compile with a separate compiler.

I believe this [1] is part of the discussion on the ARM side.

We should be consistent here and either change ARM to also emit an error or change both to emit warnings.

Changing ARM to emit an error or an on-by-default warning may break builds that didn't break before. Maybe a -Wall warning would be better? How does gcc handle this?

[1] http://reviews.llvm.org/D2255

Changing ARM to emit an error or an on-by-default warning may break builds that didn't break before. Maybe a -Wall warning would be better? How does gcc handle this?

I warning would be reasonable, but given GCC just inlines the assembly text which strikes me as them implicitly claiming this as ill-formed input as neither GCC nor clang with -fno-integrated-as have no way to ensure meaning propagation of mode changes past inline assembly, I'm leaning towards asserting a full on error over issuing a warning.

[1] http://reviews.llvm.org/D2255

I'll add the folks on D2255 and see what they think.

https://llvm.org/bugs/show_bug.cgi?id=18231

Ahha. So gcc will insert .code 16 after inline assembly calls when called form thumb mode, but nothing for inline asm in arm mode (hence my confusion). Presumably this is because the textual thumb assembly language is a subset of the arm assembly language correct code is always generated. It should probably be left as is.

Regardless this does not happen in gcc's X86 compilation for any mode, so throwing an error here seems reasonable.

I agree this looks like a good idea, and I do also think it probably ought to be an error, since the effects are likely fairly tragic if you get it wrong.

So, ARM is expected to always restore the proper thumb/arm mode after any inline asm block. But, what should (on arm) .cpu, .arch, .arch_extension, and .fpu do in inline-asm? Or .cpu, and .arch on AArch64? Or the similar things on MIPS?

IMO, those should all emit hard errors if we detect that the mode ends up being different than expected after the inline-asm block too. Perhaps the code to detect mode-switches should be done generically for all arches, after calling emitInlineAsmEnd (to give it the chance to restore the mode if appropriate)?

MIPS avoids this problem in LLVM as a side-effect of something we did to match the assembler state that GCC uses for inline assembly. We emit a '.set push' in emitInlineAsmStart() to push the current assembler state onto a stack, followed by a few '.set ...' directives to change the state to what GCC uses. Then we emit a '.set pop' in emitInlineAsmEnd() to restore the state back to what we normally use for codegen. As a result, LLVM silently discards any state changes the user forgets to undo. This differs from GCC which will assume user did the right thing and will probably end up causing assembler errors or silent miscompilations.

That said, I think this kind of check is also useful for MIPS to catch portability bugs.