[clang][asm goto][slh] Warn if asm goto + SLH

[zbrid]

Summary

Asm goto is not supported by SLH. Warn if an instance of asm goto is detected
while SLH is enabled.

Test included.

[jyu2]

Two questions:
1> What happen under SLH, will asm goto gets removed, or a runtime problem?
2> Should we emit error or warning in the Parser instead?

[zbrid]

Update to fix failing test

[zbrid]

In D79743#2034792, @jyu2 wrote:

Two questions:
1> What happen under SLH, will asm goto gets removed, or a runtime problem?
2> Should we emit error or warning in the Parser instead?

1. SLH crashes with an unhelpful error message.
2. I'd be happy to emit the warning in the Parser. Could you give me a pointer to where in the parser would be appropriate? I'm not super familiar with the clang codebase. Also why do you think the parser would be a better place? (Asking to learn since I'm new to this area)

[jyu2]

In D79743#2034945, @zbrid wrote:

In D79743#2034792, @jyu2 wrote:

Two questions:
1> What happen under SLH, will asm goto gets removed, or a runtime problem?
2> Should we emit error or warning in the Parser instead?

1. SLH crashes with an unhelpful error message.
2. I'd be happy to emit the warning in the Parser. Could you give me a pointer to where in the parser would be appropriate? I'm not super familiar with the clang codebase. Also why do you think the parser would be a better place? (Asking to learn since I'm new to this area)

Understand, no problem!
Do you mean runtime crash? If so, I think error should be emit, so that programmer can remove use of "asm goto" and recompile.

It to me, you can emit error somewhere in ParseAsmStatement when “goto” is parsed. Let me know if you have problem.

[mattdr]

Not sure we want to move this into the Parser -- SLH is a property of code generation, and I think it's possible (through LTO?) to mismatch the flags between parse and codegen.

[mattdr]

clang/include/clang/Basic/DiagnosticCommonKinds.td

251–252

I think at the UI level this feature is just called "asm goto" with no "GCC". See e.g. https://lists.llvm.org/pipermail/llvm-dev/2018-October/127239.html

Also, since this is a warning (vs. an error), we probably want to hint about the consequences of continuing despite the warning.

My attempt:
"Speculative load hardening may not fully protect functions with 'asm goto'"

252

I don't think we need to attach a DiagGroup to this. The goal would be to allow users to turn a bunch of different diagnostics on/off at once, which doesn't apply here yet.

If in the future we do need to add a DiagGroup, I think we would need to add it here before attaching it to specific warnings: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/DiagnosticGroups.td

[craig.topper]

In D79743#2035575, @mattdr wrote:

Not sure we want to move this into the Parser -- SLH is a property of code generation, and I think it's possible (through LTO?) to mismatch the flags between parse and codegen.

This is frontend code generation or "IR generation". So it should't be possible to mismatch the flags.

[jyu2]

I don’t know what consequences is of using asm goto under SLH.

In general, if asm-goto is not allowed, the diagnostic should be emitted during the parser. If asm-goto is not allowed under specified condition, the diagnostic should be emitted during sema. Diagnostic should not be emitted in the lower(codegen) in general (exception may be for target related).

[zbrid]

Do you mean runtime crash? If so, I think error should be emit, so that programmer can remove use of "asm goto" and recompile.

This would be a compile time crash. At some point the X86SpeculativeLoadHardening pass in the backend will notice asm goto is being used and give up. As far as I can tell it's hard to determine that asm goto was the root cause of that crash in the backend, so I want to emit it earlier in Clang. Does that make sense? Let me know if not :)

It to me, you can emit error somewhere in ParseAsmStatement when “goto” is parsed. Let me know if you have problem.

Thanks for the pointer! I'll send an update that emits at that point.

In D79743#2036814, @jyu2 wrote:

I don’t know what consequences is of using asm goto under SLH.

In general, if asm-goto is not allowed, the diagnostic should be emitted during the parser. If asm-goto is not allowed under specified condition, the diagnostic should be emitted during sema. Diagnostic should not be emitted in the lower(codegen) in general (exception may be for target related).

Ah okay. Asm goto isn't allowed with SLH in general, so sounds like this should be in the parser based on your comment here. Thanks for the explanation.

Thanks for the comments, everyone.

clang/include/clang/Basic/DiagnosticCommonKinds.td

251–252

I believe the DiagGroup is required for all new warnings (see: https://github.com/llvm/llvm-project/blob/master/clang/test/Misc/warning-flags.c) and I didn't notice one that fit this particular flag well. In addition I saw that adding a diag group in this way was used in the other places (like the warning right before the one I added), so I think this is an okay addition. If we add other slh related flags, we could perhaps generalize the diagnostic group to cover all those flags at that point.

Good points wrt the error message. I'll update it not to mention gcc and to explain what will happen if they use asm goto with SLH.

[zbrid]

Update to emit the warning in the parser

[zbrid]

@jyu2 and @mattdr - updated to address your comments.

[jyu2]

This looks good to me. Could you also add a test to use this new DiagGroup (-Wno-slh-asm-goto)?

Thanks.

Jennifer

[zbrid]

Add test; Update command for existing test

Also rename file to match warning flag

[zbrid]

In D79743#2044676, @jyu2 wrote:

This looks good to me. Could you also add a test to use this new DiagGroup (-Wno-slh-asm-goto)?

Thanks.

Jennifer

Done.

[jyu2]

LGTM. Please changed format for td file.

clang/include/clang/Basic/DiagnosticCommonKinds.td

250

Just need run clang-format:
def warn_slh_does_not_support_asm_goto

: Warning<"Speculative load hardening does not protect functions with "
          "asm goto">,
  InGroup<DiagGroup<"slh-asm-goto">>;

[zbrid]

ClangFormat diagnostic definition

clang/include/clang/Basic/DiagnosticCommonKinds.td

def err_asm_invalid_type_in_input : Error<
     "invalid type %0 in asm input for constraint '%1'">;
 
   def err_asm_invalid_type : Error<
     "invalid type %0 in asm %select{input|output}1">;
 
   def warn_stack_clash_protection_inline_asm : Warning<
     "Unable to protect inline asm that clobbers stack pointer against stack clash">,
     InGroup<DiagGroup<"stack-protector">>;
+
+  def warn_slh_does_not_support_asm_goto
+      : Warning<"Speculative load hardening does not protect functions with "
+                "asm goto">,
+        InGroup<DiagGroup<"slh-asm-goto">>;
 }
 
 // Sema && Serialization
 def warn_dup_category_def : Warning<
   "duplicate definition of category %1 on interface %0">;
 
 // Targets
 

clang/lib/Parse/ParseStmtAsm.cpp

if (getLangOpts().AsmBlocks && !isGCCAsmStatement(Tok)) {
     return ParseMicrosoftAsmStatement(AsmLoc);
   }
 
   SourceLocation Loc = Tok.getLocation();
   GNUAsmQualifiers GAQ;
   if (parseGNUAsmQualifierListOpt(GAQ))
     return StmtError();
 
+  if (GAQ.isGoto() && getLangOpts().SpeculativeLoadHardening)
+    Diag(Loc, diag::warn_slh_does_not_support_asm_goto);
+
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
   ExprResult AsmString(ParseAsmStringLiteral(/*ForAsmLabel*/ false));
 
   // Check if GNU-style InlineAsm is disabled.
   // Error on anything other than empty string.
   if (!(getLangOpts().GNUAsm || AsmString.isInvalid())) {

clang/test/Parser/slh-asm-goto-no-warn.cpp

+// RUN: %clang_cc1 -Wno-slh-asm-goto -mspeculative-load-hardening -fsyntax-only -verify %s
+
+void f() {
+  __asm goto("movl %ecx, %edx"); // expected-no-diagnostics
+}

clang/test/Parser/slh-asm-goto.cpp

+// RUN: %clang_cc1 -mspeculative-load-hardening -fsyntax-only -verify %s
+
+void f() {
+  __asm goto("movl %ecx, %edx"); // expected-warning {{Speculative load hardening does not protect functions with asm goto}}
+}