This is an archive of the discontinued LLVM Phabricator instance.

Add a flag to debug automatic variable initialization
ClosedPublic

Authored by jcai19 on Mar 31 2020, 1:35 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jcai19 created this revision.Mar 31 2020, 1:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 1:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jcai19 changed the visibility from "Public (No Login Required)" to "No One".Mar 31 2020, 2:18 PM
jcai19 updated this revision to Diff 254019.Mar 31 2020, 3:16 PM

Add test cases.

jcai19 changed the visibility from "No One" to "Public (No Login Required)".
jcai19 added subscribers: manojgupta, llozano, gbiv and 2 others.
MaskRay added inline comments.
clang/lib/CodeGen/CGDecl.cpp
1814

I am a bit worried about the static variable. This makes CodeGen not reusable.

clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
2

You can drop -fblocks, because the test has nothing to do with the Blocks extension.

jfb added a comment.Mar 31 2020, 3:47 PM

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.

jcai19 added a subscriber: srhines.Mar 31 2020, 4:53 PM
jfb added a comment.Mar 31 2020, 5:27 PM

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.

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 time than doing something like this. I'd much rather see bisection using pragma to apply the uninitialized attribute to multiple declarations.

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.

jcai19 updated this revision to Diff 254240.Apr 1 2020, 10:14 AM

Address some of the concerns.

jcai19 updated this revision to Diff 254242.Apr 1 2020, 10:15 AM

Remove an unnecessary line.

jcai19 added a comment.EditedApr 1 2020, 10:18 AM

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.

jcai19 marked 4 inline comments as done.Apr 1 2020, 10:38 AM
jcai19 added inline comments.
clang/lib/CodeGen/CGDecl.cpp
1814

I can't seem to find IRGenModule. Do you mean CodeGenModule by any chance? Thanks.

jfb added a comment.Apr 1 2020, 10:48 AM

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.

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't a good bisection experience. I'd rather see a begin / end bound.

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.

rjmccall added inline comments.Apr 1 2020, 11:34 AM
clang/lib/CodeGen/CGDecl.cpp
1814

Yes, sorry. The Clang and Swift frontends use slightly different names for the same concept.

jcai19 added a comment.EditedApr 1 2020, 12:08 PM
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 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.

jfb added a comment.Apr 1 2020, 12:35 PM
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 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 (or maybe we can wrap consecutive functions within the same pragma push/pop pair? I tried to verify but ran into error: attribute 'uninitialized' is not supported by '#pragma clang attribute), and automating it is more difficult. From this perspective, I think the proposed patch is complementary to pragma and not meant to replace it.

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.

In D77168#1955500, @jfb 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 to see it attempted here.

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 (or maybe we can wrap consecutive functions within the same pragma push/pop pair? I tried to verify but ran into error: attribute 'uninitialized' is not supported by '#pragma clang attribute), and automating it is more difficult. From this perspective, I think the proposed patch is complementary to pragma and not meant to replace it.

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.

vitalybuka resigned from this revision.Apr 8 2020, 6:21 PM

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?

jfb added a comment.Apr 16 2020, 3:59 PM

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).

jfb added a comment.Apr 16 2020, 8:15 PM

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.

In D77168#1988122, @jfb 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 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.

In D77168#1988122, @jfb 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 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.

jfb added a comment.Apr 17 2020, 5:14 PM

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.

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 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.

phosek added a subscriber: phosek.Apr 20 2020, 4:21 PM

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.

jcai19 updated this revision to Diff 260788.Apr 28 2020, 4:04 PM

Update baesd on comments.

jcai19 marked an inline comment as done.Apr 28 2020, 4:11 PM
  • 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.

jcai19 updated this revision to Diff 260796.Apr 28 2020, 4:39 PM

Update tests.

Harbormaster completed remote builds in B55057: Diff 260796.
jfb added a comment.Apr 29 2020, 6:36 PM

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 ? :(

jcai19 updated this revision to Diff 261595.May 1 2020, 5:35 PM
jcai19 marked 3 inline comments as done.

Update based on comments.

jcai19 marked an inline comment as done.May 1 2020, 5:37 PM
jcai19 added inline comments.
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.

jcai19 updated this revision to Diff 261597.May 1 2020, 5:58 PM
This comment was removed by jcai19.
jcai19 updated this revision to Diff 261740.May 3 2020, 11:27 PM

Issue a warning when the proposed flag is used.

Just want to follow up and see if anything is missing from the latest patch :)

jcai19 added a comment.Jun 2 2020, 2:25 PM

@jfb Does the current implementation look good? Thanks.

Do we have a mechanism bisecting pragmas? If yes, we can let that tool add #pragma clang attribute push([[clang::uninitialized]], apply_to = variable)

jcai19 added a comment.Jun 2 2020, 6:15 PM

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.

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.

If we have a mechanism bisecting pragmas, this option will not be needed.

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.

If we have a mechanism bisecting pragmas, this option will not be needed.

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.

jfb added a comment.Jun 4 2020, 8:08 AM

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.

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.

If we have a mechanism bisecting pragmas, this option will not be needed.

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.

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.

jcai19 updated this revision to Diff 269103.Jun 7 2020, 9:03 PM
jcai19 marked 4 inline comments as done.

Cover VLAs.

jcai19 added a comment.Jun 7 2020, 9:04 PM
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 the patch did not cover VLAs, which I have addressed in the latest version along other issues you brought up. Thanks.

jcai19 added a comment.Jun 7 2020, 9:09 PM

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.

jfb accepted this revision.Jun 8 2020, 10:27 AM

Two minor corrections, looks good otherwise.

clang/include/clang/Basic/DiagnosticDriverKinds.td
491 ↗(On Diff #269103)

Typo "accepts".

clang/lib/CodeGen/CodeGenModule.cpp
5973 ↗(On Diff #269103)

Could you print out the count here, instead of *?

This revision is now accepted and ready to land.Jun 8 2020, 10:27 AM
jcai19 updated this revision to Diff 269301.Jun 8 2020, 11:51 AM

Fix typos.

jcai19 marked 2 inline comments as done.Jun 8 2020, 11:52 AM
In D77168#2080296, @jfb wrote:

Two minor corrections, looks good otherwise.

Thank you for all the comments and help. I have fixed the issues.

This revision was automatically updated to reflect the committed changes.