[PATCH] D79743: [clang][asm goto][slh] Warn if asm goto + SLH

[Zola Bridges via Phabricator via cfe-commits]

zbrid marked 3 inline comments as done.
zbrid added a comment.

> 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 <https://reviews.llvm.org/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.



================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:248-249
+  def warn_slh_does_not_support_gcc_asm_goto : Warning<
+    "speculative load hardening does not support use of GCC asm goto. asm goto "
+        "detected with SLH">, InGroup<DiagGroup<"slh-asm-goto">>;
 }
----------------
mattdr wrote:
> 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'"
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79743/new/

https://reviews.llvm.org/D79743