[PATCH] D77168: Add a flag to debug automatic variable initialization

This revision was automatically updated to reflect the committed changes. Closed by commit rG4db2b7024868: Add a flag to debug automatic variable initialization (authored by jcai19). Changed prior to commit: https://reviews.llvm.org/D77168?vs=269301&id=269324#toc Repository: rG LLVM Github M

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 marked 2 inline comments as done. jcai19 added a comment. In D77168#2080296 , @jfb wrote: > Two minor corrections, looks good otherwise. Thank you for all the comments and help. I have fixed the issues. Repository: rG LLVM Github Monorepo CHA

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 269301. jcai19 added a comment. Fix typos. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/DiagnosticDriverKinds.td clang/include/cl

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb accepted this revision. jfb added a comment. This revision is now accepted and ready to land. Two minor corrections, looks good otherwise. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:491 +def err_drv_trivial_auto_var_init_stop_after_invalid_value : Error<

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. > If we have a mechanism bisecting pragmas, this option will not be needed. Yes we would have not had to start to work on this patch if such mechanism had been implemented. We can always migrate to that approach if said mechanism is implemented in the future. I think it

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 269103. jcai19 marked 4 inline comments as done. jcai19 added a comment. Cover VLAs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/D

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. In D77168#2073736 , @jfb wrote: > Can you add a test for the diagnostic firing after the correct number of > initializations? This should include a few types of auto-init, including VLAs. Thank you for the comments! It turned out

[PATCH] D77168: Add a flag to debug automatic variable initialization

MaskRay added a comment. In D77168#2070334 , @llozano wrote: > In D77168#2070083 , @MaskRay wrote: > > > In D77168#2070049 , @jcai19 wrote: > > > > > In D77168#2069783

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. Can you add a test for the diagnostic firing after the correct number of initializations? This should include a few types of auto-init, including VLAs. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:489 + "-ftrivial-auto-var-init-stop-afte

[PATCH] D77168: Add a flag to debug automatic variable initialization

llozano added a comment. In D77168#2070083 , @MaskRay wrote: > In D77168#2070049 , @jcai19 wrote: > > > In D77168#2069783 , @MaskRay wrote: > > > > > Do we have a mechanism b

[PATCH] D77168: Add a flag to debug automatic variable initialization

MaskRay added a comment. In D77168#2070049 , @jcai19 wrote: > In D77168#2069783 , @MaskRay wrote: > > > Do we have a mechanism bisecting pragmas? If yes, we can let that tool add > > `#pragma clang attribute push([

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. In D77168#2069783 , @MaskRay wrote: > Do we have a mechanism bisecting pragmas? If yes, we can let that tool add > `#pragma clang attribute push([[clang::uninitialized]], apply_to = variable)` Not that I am aware of unfortunately

[PATCH] D77168: Add a flag to debug automatic variable initialization

MaskRay added a comment. Do we have a mechanism bisecting pragmas? If yes, we can let that tool add `#pragma clang attribute push([[clang::uninitialized]], apply_to = variable)` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://revi

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. @jfb Does the current implementation look good? Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 ___ cfe-commits mailing list cfe

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. Just want to follow up and see if anything is missing from the latest patch :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 ___ cfe-c

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 261740. jcai19 added a comment. Issue a warning when the proposed flag is used. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/Diagno

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 261597. jcai19 added a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/LangOptions.def clang/include/clang/Driver/Options.td

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 marked an inline comment as done. jcai19 added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1817 + CGM.countAutoVarInit(); +} + jfb wrote: > This isn't the right place to stop auto-init: we can get past this point even > without auto

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 261595. jcai19 marked 3 inline comments as done. jcai19 added a comment. Update based on comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. I think you want to emit a diagnostic any time auto-init doesn't happen because of this new flag. With the code simplification I propose, it would be pretty hard to introduce a regression... and it someone does it wouldn't be a silent one :) Comment at:

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 260796. jcai19 added a comment. Update tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/LangOptions.def clang/include/clang/Dr

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 marked an inline comment as done. jcai19 added a comment. > - Allows bracketing as John suggested (lower / upper bounds where to stop / > start). I have made some updates to this patch based on the comments here. One question I have though is whether we should have another option for s

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 260788. jcai19 added a comment. Update baesd on comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/LangOptions.def clang/incl

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. > I'd also like to see the pragma attribute approach, as well as byte-pattern > variability as I described. I don't think auto-narrowing is the only approach > we should push people towards. Thank you for all the feedback. I agree, the pragma attribute and the byte-pat

[PATCH] D77168: Add a flag to debug automatic variable initialization

phosek added a comment. We've rolled out `-ftrivial-auto-var-init=pattern` to all of Fuchsia several months ago. In my experience, having the `pragma` would have been really valuable during the rollout stage when we've often commented out portion of code while narrowing down the issue, which is

[PATCH] D77168: Add a flag to debug automatic variable initialization

hubert.reinterpretcast added a comment. In D77168#1989973 , @jfb wrote: > I'd also like to see the pragma attribute approach, as well as byte-pattern > variability as I described. I don't think auto-narrowing is the only approach > we should push people

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. Automatic narrowing of bugs is indeed compelling, so I'd support that as long as it: - Allows bracketing as John suggested (lower / upper bounds where to stop / start). - Is implemented in a way which makes it really hard to regress the security mitigation. Maybe this requ

[PATCH] D77168: Add a flag to debug automatic variable initialization

llozano added a comment. In D77168#1988138 , @srhines wrote: > In D77168#1988122 , @jfb wrote: > > > In D77168#1988049 , @srhines wrote: > > > > > `pragma clang attribute` is

[PATCH] D77168: Add a flag to debug automatic variable initialization

srhines added a comment. In D77168#1988122 , @jfb wrote: > In D77168#1988049 , @srhines wrote: > > > `pragma clang attribute` is interesting, but how do you apply that in a > > selective fashion to local variables

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. In D77168#1988049 , @srhines wrote: > `pragma clang attribute` is interesting, but how do you apply that in a > selective fashion to local variables (especially in a way that can be > automated)? At first, I didn't think the goal for

[PATCH] D77168: Add a flag to debug automatic variable initialization

srhines added a comment. `pragma clang attribute` is interesting, but how do you apply that in a selective fashion to local variables (especially in a way that can be automated)? At first, I didn't think the goal for this should be to create a frequently used option for **most** end users, but

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. In D77168#1984109 , @jcai19 wrote: > > Right, support needs to be added, and I was wondering if you'd want to do > > this because it seems like a tool that would help you out. > > Sorry for the delay. Yes the biggest advantage of this

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. > Right, support needs to be added, and I was wondering if you'd want to do > this because it seems like a tool that would help you out. Sorry for the delay. Yes the biggest advantage of this option would be to make automating bisection much easier. Or are you suggesting

[PATCH] D77168: Add a flag to debug automatic variable initialization

MaskRay added a comment. In D77168#1955500 , @jfb wrote: > In D77168#1955450 , @jcai19 wrote: > > > In D77168#1955312 , @jfb wrote: > > > > > Do you not think `pragma` is a m

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. In D77168#1955450 , @jcai19 wrote: > In D77168#1955312 , @jfb wrote: > > > Do you not think `pragma` is a more general approach? That's what's used in > > a bunch of other cases, and I'd like t

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. In D77168#1955312 , @jfb wrote: > Do you not think `pragma` is a more general approach? That's what's used in a > bunch of other cases, and I'd like to see it attempted here. Yes I absolutely agree pragma is a great way to bisect

[PATCH] D77168: Add a flag to debug automatic variable initialization

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1814 +if (StopAfter) { + static unsigned Counter = 0; + if (Counter >= StopAfter) jcai19 wrote: > rjmccall wrote: > > srhines wrote: > > > MaskRay wrote: > > > > I am a bit wor

[PATCH] D77168: Add a flag to debug automatic variable initialization

srhines added a comment. In D77168#1955312 , @jfb wrote: > Do you not think `pragma` is a more general approach? That's what's used in a > bunch of other cases, and I'd like to see it attempted here. > If not, I agree with John that just counting up isn'

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. Do you not think `pragma` is a more general approach? That's what's used in a bunch of other cases, and I'd like to see it attempted here. If not, I agree with John that just counting up isn't a good bisection experience. I'd rather see a begin / end bound. You're also miss

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 marked 4 inline comments as done. jcai19 added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1814 +if (StopAfter) { + static unsigned Counter = 0; + if (Counter >= StopAfter) rjmccall wrote: > srhines wrote: > > MaskRay wrote: > >

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. > For example, won't you need some way of figuring out what the 1,134th > variable in a file was after you've successfully bisected? Once we know the Xth variable is causing issues, we can compare the IR or assembly of -fvar-auto-init-stop-after=X-1 and -fvar-auto-init-s

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 254242. jcai19 added a comment. Remove an unnecessary line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/AST/ASTContext.h clang/include

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 254240. jcai19 added a comment. Address some of the concerns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/AST/ASTContext.h clang/inclu

[PATCH] D77168: Add a flag to debug automatic variable initialization

rjmccall added a comment. I can understand the automation benefits of just counting the number of variables initialized so far in the translation unit without having to modify source, but if you can possibly do this with the pragma, that seems like both a more flexible tool (you can isolate mul

[PATCH] D77168: Add a flag to debug automatic variable initialization

srhines added a comment. In D77168#1953635 , @jfb wrote: > I'm not sure this is a good idea at all. We want to keep the codepath as > simple as possible to avoid introducing bugs. If a codebase sees a crash then > it's easier to bisect one function at a

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. I'd much rather see folks bisect using something like: void use(void*); #pragma clang attribute push ([[clang::uninitialized]], apply_to = variable) void buggy() { int arr[256]; int boom; float bam; struct { int oops; } oops; union { int

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 added a comment. Agreed, but having this option may save quite some time when bisecting files with many functions and stack variables, like this one . It will be much easier to write a

[PATCH] D77168: Add a flag to debug automatic variable initialization

jfb added a comment. I'm not sure this is a good idea at all. We want to keep the codepath as simple as possible to avoid introducing bugs. If a codebase sees a crash then it's easier to bisect one function at a time than doing something like this. I'd much rather see bisection using pragma to

[PATCH] D77168: Add a flag to debug automatic variable initialization

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1814 +if (StopAfter) { + static unsigned Counter = 0; + if (Counter >= StopAfter) I am a bit worried about the static variable. This makes CodeGen not reusable. ==

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 updated this revision to Diff 254019. jcai19 added a comment. Add test cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77168/new/ https://reviews.llvm.org/D77168 Files: clang/include/clang/Basic/LangOptions.def clang/include/clang/

[PATCH] D77168: Add a flag to debug automatic variable initialization

jcai19 created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Add -ftrivial-auto-var-init-stop-after= to limit the number of times stack variables are initialized when -ftrivial-auto-var-init= is used to initialize stack variables to zero or a pattern. This f