This is an archive of the discontinued LLVM Phabricator instance.

[IR] make -warn-frame-size into a module attr
ClosedPublic

Authored by nickdesaulniers on Jun 8 2021, 2:35 PM.

Details

Summary

-Wframe-larger-than= is an interesting warning; we can't know the frame
size until PrologueEpilogueInsertion (PEI); very late in the compilation
pipeline.

-Wframe-larger-than= was propagated through CC1 as an -mllvm flag, then
was a cl::opt in LLVM's PEI pass; this meant it was dropped during LTO
and needed to be re-specified via -plugin-opt.

Instead, make it part of the IR proper as a module level attribute,
similar to D103048. Introduce -fwarn-stack-size CC1 option.

Diff Detail

Event Timeline

nickdesaulniers created this revision.Jun 8 2021, 2:35 PM
nickdesaulniers requested review of this revision.Jun 8 2021, 2:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 8 2021, 2:35 PM
nickdesaulniers edited the summary of this revision. (Show Details)Jun 8 2021, 2:35 PM
nickdesaulniers added inline comments.
clang/include/clang/Driver/Options.td
2575–2582

@rsmith I wasn't super sure about this hunk of the diff. Should I not be reusing the same flag between the Frontend and the Driver? (The Driver comes after the Frontend, IIUC? Is that right?)

  • add CHECK to llvm/test/Linker/warn-stack-frame.ll
qcolombet accepted this revision.Jun 8 2021, 3:24 PM

Hi Nick,

From the backend prospective, this looks fine but you'll want someone to look at the front end part before landing that change.

Cheers,
-Quentin

This revision is now accepted and ready to land.Jun 8 2021, 3:24 PM
nickdesaulniers edited the summary of this revision. (Show Details)Jun 9 2021, 4:17 PM
rsmith added inline comments.Jun 9 2021, 4:29 PM
clang/include/clang/Driver/Options.td
2575–2582

(The driver comes before the frontend: the driver is the user-facing GCC-compatible clang binary, and the frontend is the -cc1 interface.)

What we do in other cases is to translate the driver-level (user-facing) warning flag into multiple frontend flags:

  • The driver-level flag -Wwrite-strings (in C) gets mapped into the frontend-level flags -Wwrite-strings -fconst-strings, where -Wwrite-strings only controls whether the warning is shown, and -fconst-strings only controls whether string literals have type char[N] or const char[N].
  • The driver-level flag -Wdeprecated (in C++) gets mapped into the frontend-level flags -Wdeprecated -fdeprecated-macro, where -Wdeprecated only controls whether the warning is shown, and -fdeprecated-macro only controls whether __DEPRECATED is implicitly defined by the preprocessor.

So I think the expected strategy here would probably be that the driver-level flag -Wframe-larger-than=N would get mapped into the frontend-level flags -Wframe-larger-than -fsomething=N, where -Wframe-larger-than only controls whether the warning is shown, and -fsomething=N sets the level of stack usage at which the backend triggers the warning.

nickdesaulniers edited the summary of this revision. (Show Details)
  • introduce -fwarn-stack-size
nickdesaulniers marked an inline comment as done.Jun 9 2021, 4:56 PM
  • update comment in clang/include/clang/Basic/CodeGenOptions.def
  • add < to same comment
rsmith accepted this revision.Jun 10 2021, 1:31 PM
rsmith added inline comments.
clang/test/Frontend/Wframe-larger-than.c
3

Comment needs updating.

nickdesaulniers marked an inline comment as done.
  • move driver test from clang/test/Frontend/ to clang/test/Driver/, update comment, clarify TODOs
rsmith accepted this revision.Jun 10 2021, 2:14 PM
This revision was landed with ongoing or failed builds.Jun 10 2021, 4:15 PM
This revision was automatically updated to reflect the committed changes.

FWIW, it was pointed out to me that the commit message doesn't precisely match the actual attribute name. The actual attribute name is warn-stack-size, but the commit message uses -warn-frame-size. Oh well.

dblaikie added inline comments.
llvm/test/Linker/warn-stack-frame.ll
7

If you like, you could make this a function attribute instead, that way it'd be compatible with LTO even when the value varies between compilations?

llvm/test/Linker/warn-stack-frame.ll
7

Yep, this is already causing problems for us, so I will fix that ASAP: https://github.com/ClangBuiltLinux/linux/issues/1395.