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 flag can be used
to bisect uninitialized uses of a stack variable exposed by automatic
variable initialization, such as http://crrev.com/c/2020401.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 apply the uninitialized attribute to multiple declarations.
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 script that automates the bisection process. What do you think? Thanks.
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 oof; float aaaaa; } oof; use(&arr); use(&boom); use(&bam); use(&oops); use(&oof); } #pragma clang attribute pop
This should be easy to add support for.
Certainly one function at a time is preferable, but that's not always possible. I do think that this would be a useful feature for making bisection easier, since automating the injection of pragmas into code seems a lot more challenging. Of course, the pragma approach is nice for other reasons (letting people carve out entire functions for initialized/uninitialized, but I think that is orthogonal to the debugging aspect.
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1814 | The counter could exist in ASTContext instead. |
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 multiple variables) and one that will produce more easily-interpretable results. 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?
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1814 | IRGenModule would be the more appropriate place. |
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-stop-after=X to figure out which variable it is, or I can print out the variable name and the enclosing function if it is preferred.
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1814 | I can't seem to find IRGenModule. Do you mean CodeGenModule by any chance? Thanks. |
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 missing the auto-init in initializeAlloca.
I agree that begin/end is actually better.
My experience has been that you really don't want to be modifying source files if you can avoid it, as that is harder to automate in a script. And you do want to script this kind of debugging because it isn't a compiler crash, it's often a runtime issue that might take longer to reproduce/etc., so I think that it is preferable to remove the human (error prone) element of adding/tweaking pragmas. I did say that the pragmas are useful in other ways, but I don't think that it is effective for everything.
You're also missing the auto-init in initializeAlloca.
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1814 | Yes, sorry. The Clang and Swift frontends use slightly different names for the same concept. |
Yes I absolutely agree pragma is a great way to bisect and will work in many scenarios, and the result is clear once the bisection is done. But like @srhines said, doing this one function at a time is preferable but not always possible, and automating it is more difficult. From this perspective, I think the proposed patch is complementary to pragma and not meant to replace it.
If not, I agree with John that just counting up isn't a good bisection experience. I'd rather see a begin / end bound.
Thank you for the feedback! I will start looking into it.
You're also missing the auto-init in initializeAlloca.
Thanks I will address this in next patch.
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.
My understanding is that we can invent a compiler option to add #pragma clang attribute push for declarations satisfying some criteria (e.g. first N)? No source is modified. This option can potentially be useful for other attributes which are orthogonal to their functionality (concrete examples: sanitizer/XRay attributes). If we go that route, I agree that this proposed -ftrivial-auto-var-init-stop-after= does not seem that useful.
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 we should do it differently, like a tool that can automatically inserts macros in the source code?
I suggest implementing support for this:
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 oof; float aaaaa; } oof; use(&arr); use(&boom); use(&bam); use(&oops); use(&oof); } #pragma clang attribute pop
pragma clang attribute is already a feature, we just need to teach it about this one attribute.
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 I do think that it could be quite useful for more folks when debugging, especially if it is easy to automate (which optimization-fuel approaches are, while pragmas are not).
__attribute__((uninitialized)) for more selectiveness :)
Of course, not automated. In general we don't really automate compiler things of this sort, say UBSan. OptRemarks is the best we really have to dig into these things. One other option would be to truly randomize the init pattern: emit a handful of different byte patterns, and then see the crash caused by a different pattern, and track it back to which local variable got that byte pattern.
That's just it though. This technique is tried and true in other compilers/tools. Why should we let past history dictate whether to facilitate automation in the present/future? Rather than having to "track it back", you can let the tooling do it for you. It gets you at least a breakage related to this transformation, and the best part is you can fix that one, and then run it again to chase down further issues. All without modifying source code. I feel like I can't be the only one who has used these kinds of tools for debugging transformations before, hence why I feel so invested in making this easy for everyone (compiler devs and regular users). UBSan isn't as relevant as it can provide diagnostics for the particular instance in which it was tripped. If we wanted to do something similar for initialization, we would have to mark the uninitialized value and track down any potential use of it, which seems like a lot more complexity and decreased performance too.
I totally agree with the value of automatic triaging of compiler transformations. My team uses techniques like that quite often. It may be relatively easy to triage a failure caused/exposed by a compiler transformation when you are applying to a single package that you have some familiarity with. It is totally different when you are trying to apply a new compiler transformation to a platform made up of hundreds (or thousands) of packages you are not familiar with. If this feature becomes more popular, a lot of compiler users will benefit from having mechanisms to automatic triage initializations that change program behavior. This is useful not only for compiler developers but also for compiler users.
The pragma attribute push functionality is useful but I see that as complimentary to what this change is proposing. It allows developers more flexibility on what gets the transformation applied and a larger granularity but it is not as convenient for bisection since it needs source code changes.
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 requires emitting a diagnostic when auto-init isn't applied because of the flag.
- Is thorough (i.e. it covers all auto-init, for example initializeAlloca was missing).
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.
I also would like to see the pragma attribute in addition to the command-line option. The attribute is closer to the source code even for debugging purposes. If it is not too finicky, it might be usable even with relatively naïve text bisection tooling.
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 basically a strawman version of the pragma. Similarly, the variable byte pattern would have also been a great timesaver.
I don't think we ever had a use case for the proposed flag, which doesn't mean that it's not useful, but I don't know how to use it effectively. Specifically, every time I was debugging an uninitialized variable issue, I'd know which file and often even which portion of the file the bug is in. In our build, we always apply flags to the entire target, not individual files, so to use the proposed flag, I'd have to modify the build to extract a single file into a separate target in order to apply the flag only to that file which seems comparatively intrusive to inserting the pragma. Is that something you could comment on? Do you have an existing experience of using this flag in your build? What's your strategy for using it?
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-pattern approach are versatile, and great to have along the proposed approach. Will work on that and iterate on this patch based on the feedback.
Do you have an existing experience of using this flag in your build? What's your strategy for using it?
Thank you for sharing your experience. It's great to know other people are facing similar challenges. I used this flag to debug an issue while enabling -ftrivial-auto-var-init=pattern on ChromeOS kernel 4.14. I used a bisection tool that traced the issue into the file of interest, but I did not know which part of the code caused the issue. That file had many small functions and local variables so it would be time consuming to manually fiddle with each variable to find out the root cause. On Linux kernel, it's easy to add build flags to each individual files, and ChromeOS provided a tool to update the OS on the test device, so each time I increased the counter with the proposed option and rebuilt the kernel, I updated the test device with the new kernel to verify if the issue still reproduce. Eventually it turned out some variable was used before [initialized](crrev.com/c/2020401). It seems your scenario is slightly different, in the sense you already know which portion of the code is problematic, in that case a pragma-driven approach should work perfectly.
In our build, we always apply flags to the entire target, not individual files, so to use the proposed flag, I'd have to modify the build to extract a single file into a separate target in order to apply the flag only to that file which seems comparatively intrusive to inserting the pragma. Is that something you could comment on?
I am not familiar with Fuchsia build system, it does sound more complicated if you have to create a separate build target to just pass the flag. However, you will need this flag most likely when you run into an issue and you do not have much idea what might have been the cause, so I assume this will not happen that often :). And IMO the time saved from automating the bisect process should more than enough cover the extra effort for creating the build targets in those cases.
- 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 starting point. Now that we implement the pragma support at https://reviews.llvm.org/D78693, bracketing can be done with pragma of uninitialized attribute. For bisection, we do not really need to specify a starting point. Not having it also means less change to the code path of automatic variable initialization, and therefore a less chance of introducing bugs.
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1814 | Thank you for the confirmation! Sorry I missed your reply earlier. |
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 :)
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
84 ↗ | (On Diff #260796) | I think this logic is about right, but it seems like something you'd want to use as a two-liner instead. Maybe something like: if (autoInitStopCounter()) return; This would check the value is set, check its value against the counter, and increment the counter. |
clang/lib/CodeGen/CGDecl.cpp | ||
1817 | This isn't the right place to stop auto-init: we can get past this point even without auto-init. As-is you're introducing a change in the regular language. I hope that some tests for whatever happens below would break if you hack your flag to always stop auto-init for any value. i.e. just on your machine, run the entire test suite with this forcibly on. Only auto-init tests should fail, the others should all still pass. | |
clang/lib/CodeGen/CodeGenModule.h | ||
305 ↗ | (On Diff #260796) | This is reachable from a context where CodeGenModule is const ? :( |
clang/lib/CodeGen/CGDecl.cpp | ||
---|---|---|
1817 | I tried to always return and it seemed only auto-init tests failed while running check-clang. But I agree this is not the right place. I have moved this piece of code. Thanks! | |
clang/lib/CodeGen/CodeGenModule.h | ||
305 ↗ | (On Diff #260796) | I copied it from my previous iteration and forgot to remove the keyword. Thanks for catching that. |
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. The whole point of having this patch is to add the ability to bisect programmatically, along the approach of using pragma as introduced in https://reviews.llvm.org/D78693.
I think you are coming late into the discussion. If you read before, We had already agreed that having mechanisms for automatic triaging is "very compelling". The change suggested here just makes automatic triaging much easier at a very low cost.
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.
clang/include/clang/Basic/DiagnosticDriverKinds.td | ||
---|---|---|
489 ↗ | (On Diff #261740) | I don't think this is sufficiently clear: automatic variable initialization is now disabled because ftrivial-auto-var-init-stop-after has reached its limit. |
clang/lib/CodeGen/CGDecl.cpp | ||
1686 | I'd rather repeat this than fallthrought and repeat the condition in if/else. | |
clang/lib/CodeGen/CodeGenModule.h | ||
1390 ↗ | (On Diff #261740) | The first time this returns true is when the diagnostic should be emitted. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
3094 | Not here. |
I expressed my opinion very early in https://reviews.llvm.org/D77168#1955507 If other reviewers find this useful even if we have a generic framework, I will not object.
Thank you for the comments! It turned out the patch did not cover VLAs, which I have addressed in the latest version along other issues you brought up. Thanks.
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 should not be too difficult.
I'd rather repeat this than fallthrought and repeat the condition in if/else.