Page MenuHomePhabricator

Improve the diagnostic of DiagnosticInfoResourceLimit (and warn-stack-size in particular)
ClosedPublic

Authored by MaskRay on Jun 21 2021, 1:06 PM.

Details

Summary

Before: warning: stack size limit exceeded (888) in main (the limit (-Wframe-larger-than) is not mentioned)
After: warning: stack frame size (888) exceeds limit (100) in function 'main'

Diff Detail

Event Timeline

MaskRay created this revision.Jun 21 2021, 1:06 PM
MaskRay requested review of this revision.Jun 21 2021, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 1:06 PM

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

MaskRay added a comment.EditedJun 21 2021, 1:25 PM

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

This diagnostic is emitted by clang (clang/lib/CodeGen/CodeGenAction.cpp).

-Wframe-larger-than= also records the module flag metadata "warn-stack-size" which can be useful for LTO.

% fclang -Wframe-larger-than=100 a.c  -o x          
a.c:1:5: warning: stack frame size of 888 bytes in function 'main' [-Wframe-larger-than]
int main(){
    ^
1 warning generated.
% fclang -Wframe-larger-than=100 -flto=thin -fuse-ld=lld a.c -o x
ld.lld: warning: stack size limit exceeded (888) in main

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

This diagnostic is emitted by clang (clang/lib/CodeGen/CodeGenAction.cpp).

-Wframe-larger-than= also records the module flag metadata "warn-stack-size" which can be useful for LTO.

% fclang -Wframe-larger-than=100 a.c  -o x          
a.c:1:5: warning: stack frame size of 888 bytes in function 'main' [-Wframe-larger-than]
int main(){
    ^
1 warning generated.
% fclang -Wframe-larger-than=100 -flto=thin -fuse-ld=lld a.c -o x
ld.lld: warning: stack size limit exceeded (888) in main

Can we try to make the FE match what's printed during LTO? It's odd that they differ, IMO. Also, GCC prints the threshold value, as does our backend; but the frontend still does not.

GCC trunk: warning: the frame size of 80 bytes is larger than 0 bytes [-Wframe-larger-than=]
Clang trunk: warning: stack frame size of 88 bytes in function 'bar' [-Wframe-larger-than]

looks like the frontend doesn't use this particular text at all?

This diagnostic is emitted by clang (clang/lib/CodeGen/CodeGenAction.cpp).

-Wframe-larger-than= also records the module flag metadata "warn-stack-size" which can be useful for LTO.

% fclang -Wframe-larger-than=100 a.c  -o x          
a.c:1:5: warning: stack frame size of 888 bytes in function 'main' [-Wframe-larger-than]
int main(){
    ^
1 warning generated.
% fclang -Wframe-larger-than=100 -flto=thin -fuse-ld=lld a.c -o x
ld.lld: warning: stack size limit exceeded (888) in main

Can we try to make the FE match what's printed during LTO? It's odd that they differ, IMO. Also, GCC prints the threshold value, as does our backend; but the frontend still does not.

How about warning: stack frame size (88) exceeds limit (80) in function 'warn' for "warn-stack-size" (currently a module flag metadata, will soon become a function attribute)?

I can make the clang diagnostic match this style, too.

How about warning: stack frame size (88) exceeds limit (80) in function 'warn'

Sure. Would you mind adding the before/after text to the commit description as well, please?

MaskRay retitled this revision from Improve the diagnostic of DiagnosticInfoStackSize (and warn-stack-size in particular) to Improve the diagnostic of DiagnosticInfoResourceLimit (and warn-stack-size in particular).Jun 21 2021, 2:39 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay updated this revision to Diff 353497.Jun 21 2021, 2:45 PM

stack size -> stack frame size

The former is ambiguous (it could mean the full stack size).

Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 2:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Jun 21 2021, 2:45 PM
clang/test/Misc/backend-resource-limit-diagnostics.cl
4

The value of the limit is still not printed though.

Oh, we don't print (0) if the limit is 0? Perhaps add a --check-prefix and additional RUN line that exercises a non-zero limit from the frontend?

This will need to be rebased on https://reviews.llvm.org/rG8ace12130526f450c822ca232d1f865b247d7434 and the tests updated, potentially.

MaskRay added inline comments.Jun 21 2021, 3:55 PM
clang/test/Misc/backend-resource-limit-diagnostics.cl
4

0 means the diagnostic does not convey the limit information. It needs more refactoring to make this happen... and probably should not be the job of this patch...

nickdesaulniers accepted this revision.Jun 21 2021, 4:01 PM
This revision is now accepted and ready to land.Jun 21 2021, 4:01 PM
nickdesaulniers accepted this revision.Jun 21 2021, 4:47 PM
This revision was landed with ongoing or failed builds.Jun 22 2021, 9:55 AM
This revision was automatically updated to reflect the committed changes.