Page MenuHomePhabricator

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

Authored by zbrid on Mon, May 11, 1:35 PM.

Details

Summary

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

Test included.

Diff Detail

Event Timeline

zbrid created this revision.Mon, May 11, 1:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 11, 1:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jyu2 added a comment.Wed, May 13, 11:58 AM

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 updated this revision to Diff 263819.Wed, May 13, 12:18 PM

Update to fix failing test

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 added a comment.Wed, May 13, 1:43 PM

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.

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 added inline comments.Wed, May 13, 6:16 PM
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

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 added a comment.Thu, May 14, 10:44 AM

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 marked 3 inline comments as done.Mon, May 18, 1:07 PM

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.

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 updated this revision to Diff 264740.Mon, May 18, 3:57 PM
zbrid marked an inline comment as done.

Update to emit the warning in the parser

zbrid added a comment.Mon, May 18, 3:59 PM

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

jyu2 added a comment.Tue, May 19, 11:58 AM

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

Thanks.

Jennifer

zbrid updated this revision to Diff 265081.Tue, May 19, 4:22 PM

Add test; Update command for existing test

Also rename file to match warning flag

zbrid added a comment.Tue, May 19, 4:24 PM

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

Thanks.

Jennifer

Done.

jyu2 accepted this revision.Tue, May 19, 5:19 PM

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">>;
This revision is now accepted and ready to land.Tue, May 19, 5:19 PM
zbrid updated this revision to Diff 265282.Wed, May 20, 9:43 AM

ClangFormat diagnostic definition

zbrid marked an inline comment as done.Wed, May 20, 9:44 AM
This revision was automatically updated to reflect the committed changes.