[PATCH] D135488: [codegen] Display stack layouts in console

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

Sorry, it took me a while to circle back to this.

In D135488#3931603 <https://reviews.llvm.org/D135488#3931603>, @dblaikie wrote:

> I'm not sure we'd need/want to optimize a diagnostic experience for a codebase that's got a lot of latent warnings. If people have lots of existing warnings they should probably turn the warning off (in those instances, at least) & I think saying "this function has a frame that's too big" without any info is pretty hard to act on, so it doesn't seem totally outside the realm of good diagnostics for this particular diagnostic to print a lot of info to help a user act on it/fix the issue when they get it. If most of the time you see this warning and want to fix it, you had to run your build again with an extra flag - I'd say that was a bad diagnostic experience, we should give them enough info the first time around.

On the topic of how frequent warnings should be, it's important to keep in mind that a function with a large stack frame will generate a diagnostic in every caller who has inlined that function. So even if you were warning free in the last build a single function using too much stack could generate a lot of diagnostics.

> If most of the time you don't need this info - yeah, that's something we should figure out, how to provide the right amount of info to be actionable, but in this case I suspect more-often-than-not you want some kind of report/breakdown. If it's a case of not having a way to make it more targeted/actionable and we just have two options ("too terse" and "too verbose") fairly evenly split (it's not clear that most of the time one or the other is the right answer) - I guess two different warning flags or some kind of modifier flag could be suitable. I guess we have that for template recursion things, maybe? Where you can ask if you want the full expansion, but by default we give you a summarized one, skipping expansions we don't think are relevant (I might be misremembering).

While I agree that running the build again isn't ideal, I don't' see a good way to balance the need for clang to report concise errors with the need for more information in this case.  Reading D127923 <https://reviews.llvm.org/D127923>, I think it's clear that many of clang's maintainers would like to keep diagnostics concise if possible. I should also note that we already give some minimal context to `-Wframe-larger-than` diagnostics by printing the breakdown of the stack usage between spills, program variables, and the UnSafeStack.

I think the right approach in this case is to allow developers to opt into the behavior when needed, so your suggestion of gating it behind a flag seems like a good way to express that. I also think the diagnostic is useful enough on its own to warrant use outside of `-Wframe-larger-than`, despite being very useful when triaging those type of warnings. I guess that means I should take a harder look at plumbing this through the remarks infrastructure, even if I don't love the output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135488