Page MenuHomePhabricator

[clang] Modify diagnostic level from err to warn: anyx86_interrupt_regsave
ClosedPublic

Authored by mibintc on Apr 14 2021, 2:20 PM.

Details

Summary

I got a bug report that https://reviews.llvm.org/D97764 which introduced this diagnostic, was causing problems in interrupt service routines, complaining about functions like abort, exit, or the x86intrin routines (that are really just inlined asm). I think modifying the diagnostic level from error to warn is the safest way to deal with the problem. Checking "always inline" attribute on the called function isn't itself enough without knowing that the called function is leaf. (Separately, perhaps x86intrin functions should be decorated with attribute 'no_caller_saved_registers' in their declarations.)

Diff Detail

Event Timeline

mibintc created this revision.Apr 14 2021, 2:20 PM
mibintc requested review of this revision.Apr 14 2021, 2:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 2:20 PM
rsmith added a subscriber: rsmith.Apr 14 2021, 4:40 PM
rsmith added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
299

Please don't add warnings directly to -Wextra; this should have its own flag so that people who want to do this can turn the warning off without turning off all of -Wextra.

mibintc updated this revision to Diff 337731.Apr 15 2021, 5:49 AM

I removed the diagnostic from InGroup<Extra>, that's the only change from previous revision

mibintc marked an inline comment as done.Apr 15 2021, 5:57 AM
mibintc added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
299

Thanks, I had thought about removing InGroup<Extra> before uploading to review but hurrying at the end of my work day and forgot. I was expecting the lit test to fail and I'd have to add Wextra onto the RUN line but I didn't need to do that, the diagnostic was produced without enabling. Anyway I got rid of it like you asked.

BTW the only other diagnostic in this file marked Wextra is the arm diagnostic. That was added in https://reviews.llvm.org/D28820 "warn_arm_interrupt_calling_convention"

aaron.ballman added inline comments.Apr 15 2021, 8:39 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
295–299

You still need to add the diagnostic to a diagnostic group, otherwise users don't have a way to selectively disable the diagnostic.

Feel free to pick a different group for the -W flag; I just took a stab at a possible name.

This should fix the failing CI pipelines too.

mibintc updated this revision to Diff 337789.Apr 15 2021, 9:23 AM
mibintc marked an inline comment as done.

I added the InGroup rule for the new warning diagnostic like Aaron requested

This revision is now accepted and ready to land.Apr 15 2021, 9:27 AM