Fix for issue 283 (round 2)

[sroy4899]

Creating a new PR in order to not have to merge (as much) down the line.
Addressed the main difficulties with the earlier PR, namely that compatible Checked C types and normal types would have been rejected. Also added support for failing gracefully when multiple definitions of the same function are offered.

The current solution examines the length of the CVars (i.e. the general "structure" of the pointers as opposed to types).

In addition to normal code-review, one consideration I also wanted to bring up before we merge this PR:
Many of the multi-file regression tests make use of helper functions that appear in both files (i.e. add1, sub1, etc). This isn't really an issue, since I can just re-run my testgenerator script to change these definitions into prototypes, but I wanted to make sure that this is what we wanted before changing ~87 tests. Namely, it's the case that we want to fail when there are multiple definitions of the same function, even if they are both "compatible" with one another (or in this case, identical), right? If so, after code-review, I can change these tests to make sure we get no failures and then merge.

[mattmccutchen-cci]

FYI, I ran across this page with report_fatal_error or ExitOnError, which might be more idiomatic, but I don't think it's important.

[mwhicks1]

That's interesting. I find the ExitOnError explanation strangely hard to comprehend. Is it saying that instead of reporting a diagnostic and then calling exit, you should declare an ExitOnError class and then somehow invoke it when you have a fatal error? What about other warnings/errors that you might emit? We are now using clang::DiagnosticsEngine but I don't see that mentioned here. Your link is to LLVM stuff, not clang stuff, so perhaps that's the reason.

[mattmccutchen-cci]

In general, LLVM code is supposed to recover after issuing a diagnostic. This is a special case: I think we would report the diagnostic about the conflicting declaration (with the same message we envision from an enhanced version of 3C capable of recovering from this situation) and then exit with a message stating that such recovery is not (yet) implemented. Here we would use report_fatal_error. ExitOnError can be used to make code more concise when calling a function that is already designed to return an LLVM Error object containing an appropriate message.

Your link is to LLVM stuff, not clang stuff

My understanding is that guides like this apply to the whole LLVM umbrella, including Clang. In fact, this guide contains one mention of Clang.

Again, none of this is important for this PR, but it may be worth knowing.

[mwhicks1]

Why is this here? I would think that if you start with ReasonFailed as "" at the very beginning, then it will just stay that way. You shouldn't have to reset it. Same with the case below.

[sroy4899]

Yes, you're right. The reason I reset it here though is because if you take a look at 1695, I set ReasonFailed to be "for return value." I do this so that if we ever error out, the error message will be descriptive enough to indicate that the return value is the culprit here. If I didn't reset ReasonFailed to be "", then when we pop back up, then insertIntoExternalFunctionMap will detect that the string has changed (from "" to "for return value") and then issue an error, when an error really shouldn't be fired. It's admittedly very hacky and is a direct result of me trying to multi-purpose this variable as a boolean and as a string, so if we think it's cleaner to treat them separately, I can make that change.

[mwhicks1]

I don't follow this. Couldn't you set the error message in 1697? I.e., change the above code to

ReturnVar->mergeDeclaration(From->ReturnVar, I, ReasonFailed);
if (ReasonFailed != "") {
  ReasonFailed += "for return value"; 
  return;
}

or something like that? I worry that if you are resetting the value in various places that (a) you are confusing the algorithm a little bit, but more importantly (b) you might cause a failure to get inadvertently missed.

[sroy4899]

Yes, this seems much better. Fixed in most recent commit (yet to push, since I want to report back on changing the error to warning below)

[mwhicks1]

Instead of exiting here, just issue a warning, the way it was done before?

[mwhicks1]

Or, if we do that, perhaps that doesn't solve the libArchive problem (don't remember)?

[sroy4899]

The libarchive problem will actually just be fixed via the current stuff in mergeDeclaration, so we can issue a warning here and still have things work according to plan. The reason I decided to fail here is because we decided that was the correct solution during status.
You noted on slack that maybe failing when we see this is fine since the files won't get rewritten properly anyway. It was my understanding that rewriting doesn't suffer overly with this problem. In fact, now that you mention it, I'm fairly certain libtiff relies on this warning, since @aaronjeline added this stuff in when he was solving the libtiff crash ages ago. My understanding is that though we have these duplicate functions, we still get to correctly rewrite a bunch of other functions without incident, so I think reverting to warning is the right solution.

But, w.r.t. question 2 below, I'll double-check how the rewriter handles this (if it were a warning) and report back, and if you agree that we should revert back to warning, then I can then add the code to make sure that the prototypes are compatible with one another.

[mwhicks1]

Sounds good. Please do try out an example with two definitions of the same function in different files and let's see how rewriting might be affected. Also see what happens if both versions have different types vs. the same types.

[sroy4899]

Alright, tried out some wacky combinations, including functions that flow into other (unaffected, i.e. non duplicated functions) to see how rewriting performs, and I think it does the right thing from what I've seen. In the case where the prototypes are identical, both functions get converted in the exact same way, but when their types are different, they both become marked wild (in addition to the functions they flow to), but they retain their signatures pre-conversion (i.e. the resulting pair of files still compiles).

[mwhicks1]

I think we just agreed in our face-to-face meeting that we should die if there are multiple definitions. It's asking for trouble to keep going ... (just make the tests' duplication functions static)

[mwhicks1]

Looks good! I have two questions to look at:

• Whether you need to reset ReasonFailed in the code (I'm thinking that shouldn't happen)
• Whether the presence of two definitions could be a warning rather than failure

For the latter it may be that the answer is no. Or, if we want that to be OK, you should add code to confirm that the prototype is the same for both. If you made it a warning, what would actually happen? Would both definitions get rewritten?

[sroy4899]

Ok, just committed. Took a bit longer than expected because I had to rework some of the tests, but I think it's in a good place now.

[mwhicks1]

This looks good. One question: Did you only add static for those cases where there was a conflict otherwise? I.e., for multi-file tests? I'd prefer we use non-static for just single functions. But, if this is a pain to change, and/or it only affects a few tests, we can leave it.

[sroy4899]

Ah yes, I added static on all autogenerated tests. I don't think it should be too hard to change it back for the tests that are single-file, but I'll spend the hour before class on it, and report back.

[sroy4899]

Turns out it's not very complex at all! 😄