Fix for issue 283 (Failing gracefully) #308

sroy4899 · 2020-10-28T16:07:27Z

A fix for #283
Closes #283

Some considerations:

I still have yet to add tests. (I have them written out, but clearly, I expect them to fail. While I could add XFAIL to them, seeing any sort of FAIL, even if it's expected, always makes me panic, so I'm trying to find a way to get it to still PASS. At any rate, my code is far from perfect, so I thought I could meddle with testing stuff while the code review was taking place so I don't delay this any longer.)
Most of this code is practically verbatim from our (me, Mike, Matt) conversation in the issue, but I decided to pass around a string instead of a boolean so I could knock out the error message and the fail flag together. It's understandably a little uglier, so if we want to change that, I can do so.
I played around with the error message signalling quite a bit (it's pretty fun), and we could make the error messages really cool (i.e. show both of the conflicting FunctionDecls across files with tags on the offending variable) if there were some way to access the underlying FunctionDecl from a ConstraintVariable. I debated adding a Decl field to PVConstraint and FVConstraint, but I thought it might be too invasive a change for something that might not matter as much to warrant it. (But once again, if we decide we want to, I can make this change).
Important Question: With this change we have 5 regression test failures (because these tests #include <stdio.h> but also redeclare malloc using the checked version (with the itypes). Before, clang would complain about an incompatible redeclaration but as a warning, but now I've changed it to an error). What should our way forward from this one be? To make an exception for standard functions in our code, or instead change the tests so this redeclaration doesn't occur?

mwhicks1 · 2020-10-28T20:58:08Z

clang/lib/CConv/ProgramInfo.cpp

+          DiagBuilder.AddTaggedVal(Pointer, Kind);
+          DiagBuilder.AddString(ReasonFailed);
+        }
+        if(MergingFailed) {


I'm confused about this. Does just doing AddString to DiagBuilder above cause the message to be printed? If not, then this conditional will eat the message by causing the process to die, right?

It doesn't by itself. The way this works is as you observed. When the Diagnostic Builder goes out of scope, the associated error is output! The reason why I have a separate (but identical) conditional to exit is that since the diagnostic only fires when it goes out of scope, if I have the call to exit in the same block, the program will exit like expected, but no diagnostics will be provided. A bit hacky, but it's the only thing that seems to work if we want to immediately exit and not "soldier through."

I don't think we can just exit. This is in library code, and may be part of clangd. There needs to be some other way to abort execution. I wonder if there are other clangtools that might provide a model for what to do...

Mind clarifying what's wrong with exit()? I did a quick Google search for standard methods of termination with clang and llvm as keywords, and it seems std::terminate() is a function that's used internally that might be of use. But I don't think the intent of your comment was "replace the call to exit with the call to something else that does the same thing", but rather, that exiting abruptly might break something? If so, do you know what it breaks? I'm not too familiar with how clangd could complicate things.

clangd is a server ("clang daemon" == server running all the time). So the point is that if just do exit in the middle of our library, linked to clangd, then the clangd abruptly terminates. Instead, we just want to terminate our analysis, not terminate the whole program that called into it.

This might mean we have to do a little restructuring of the tool itself. Maybe @jackastner or @mattmccutchen-cci have ideas about how this could be done.

Without being familiar with the code (and not wanting to drop everything to try to become familiar with it right now): I don't know what the call stack for our analysis looks like, and it seems it might be a lot of work to unwind it safely and return to clangd except perhaps by throwing and catching a C++ exception (if that's acceptable in our code; I don't see any use of C++ exceptions so far). In any case, if 3C's clangd mode maintains any data structures incrementally across analysis attempts, we need to make sure the data structures can recover from termination of the analysis (due to inconsistent merged declarations or any other cause of termination we may add in the future).

If that isn't enough of a hint, I guess you have to wait for @jackastner.

I'm not sure how easy/hard it is, but you might look at the structure of the Checked C typechecker, which is I think in clang/lib/Sema/*.cpp (at least in part); it extends the existing C typechecker. That code must have the same issue: If things go south they want to signal an error, but they also want to exit gracefully. Maybe they use exceptions to do this, not sure.

mwhicks1 · 2020-10-28T21:09:06Z

clang/lib/CConv/ConstraintVariables.cpp

-                                                 ProgramInfo &Info) {
+                                                 ProgramInfo &Info,
+                                                 std::string &ReasonFailed) {
+  if (FromCV->getOriginalTy() != this->getOriginalTy()) {


I think this check is probably too strong, which is why you are getting regressions. None of the existing tests should fail. The existing types can be different, but only if they are compatible. What mergeDeclaration is doing is taking two prototypes that are compatible and merging them together into one.

Compatibility is a loose concept in my mind, but it basically boils down to the two functions having the same number of arguments and the same kinds of pointer for each, with the only difference whether one might have an itype where the other doesn't, or one might have a checked type and the other an unchecked one (e.g., _Ptr<int> vs. int *).

Maybe @aaronjeline can comment based on what he knows.

It might be that this is not the right place to put this check, i.e., that you should interleave the check in with the merging process, and check for # parameters, types, etc. as you go and then fail there.

The possibility that the tool might be run on a file that already has Checked C types in it never crossed my mind, so that's definitely a vulnerability I wasn't accounting for. At any rate, I also think this is too strict, so I'll do a bit more digging later to find a way to get it to be robust.
I know you said compatibility is a loose concept for you, but if you (or anyone reading this right now) could answer a couple of questions, it would give me some direction on how to handle this:

Is it the case that if the base types are different, we should always fail? (i.e. char vs int. These can be pointers, be _Ptrs, or whatever they want to be. But if we observe that one of them is an int and another is a char, is there ever a case in which we want to allow it to happen? If there is no such case, we can detect their base types, so that simplifies things a bit. (Although not by much, because it's possible to have typedefs or macros that are compatible i.e. size_t vs uint and it's not apparent to me that the BaseType calculation in our code unfolds them)

Is it always the case that different pointers should be rejected (by this I mean, if the i-th argument in two functions are int** and int* respectively)?

The way you can check some of these things is to make two prototypes and put them in the same file and run the Checked C clang (not our tool, the compiler) and see what happens. If it gives you an error, then this is bad. If it says nothing, good. If a warning, also "good." For example, for your last question, if I use this file:

void foo(int *x); void foo(int **x);

I get this outcome:

a.c:2:6: error: conflicting types for 'foo' void foo(int **x); ^ a.c:1:6: note: previous declaration is here void foo(int *x); ^ 1 error generated.

whereas if I do this:

void foo(int *x); void foo(int *x: itype(_Ptr<int>));

The compiler is happy with it.

BTW, to see if a Checked C type is compatible with a legacy type, you can do put them both in an itype. For example, to see if int ** is compatible with _Ptr<int>, you can write:

void foo(int **x: itype(_Ptr<int>));

Then pass this to Checked C clang. It doesn't like it, as we would expect:

a.c:2:25: error: mismatch between interface type '_Ptr<int>' and declared type 'int **' void foo(int **x: itype(_Ptr<int>)); ^ 1 error generated.

Oh, this is a game changer in terms of debugging by cases. Thanks!

For right now, at least on the int** vs int* debacle, the difference appears to be pretty clear. (In that if you do getCVars() for both of them, the number of CVars for the first will be two (one to represent the outer pointer, one to represent dereferencing the outer pointer to get the inner pointer), and the number of CVars for the second will be 1 (there's only one pointer)).
I'll probably have to do a bit of a code review in addition to looking at the Checked C typechecker to ensure that the above is an invariant (i.e. different lengths iff incompatible).

I think that's probably an invariant. @jackastner might be able to comment on that. For a while it wasn't, but then I think we decided to try to make it one, but not sure if we finished that work.

mwhicks1

I think this is not quite ready. See comments.

sroy4899 · 2020-11-06T21:57:15Z

Please refer to the latest #330 . Closing this one.

fix

6f5a74c

mattmccutchen-cci mentioned this pull request Oct 28, 2020

Rename CConv, etc. to 3C #299

Merged

2 tasks

mwhicks1 reviewed Oct 28, 2020

View reviewed changes

mwhicks1 requested changes Oct 28, 2020

View reviewed changes

sroy4899 closed this Nov 6, 2020

sroy4899 deleted the iss283 branch November 16, 2020 20:09

Fix for issue 283 (Failing gracefully) #308

Fix for issue 283 (Failing gracefully) #308

Uh oh!

Conversation

sroy4899 commented Oct 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwhicks1 left a comment

Choose a reason for hiding this comment

Uh oh!

sroy4899 commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants