Skip to content

Fix for issue 283 (round 2) #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 16, 2020
Merged

Fix for issue 283 (round 2) #330

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac34173
addressed comments, added three multi-file tests
sroy4899 Nov 6, 2020
ed45632
addressing further comments from status
sroy4899 Nov 6, 2020
0c88fb6
fixes with code, status, and tests
sroy4899 Nov 12, 2020
d8f6695
fixing single file tests to be non-static
sroy4899 Nov 12, 2020
afffadf
Merge remote-tracking branch 'origin/master-post-microsoft' into fail…
sroy4899 Nov 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions clang/include/clang/3C/ConstraintVariables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ class ConstraintVariable {

// Update this CV with information from duplicate declaration CVs
virtual void brainTransplant(ConstraintVariable *, ProgramInfo &) = 0;
virtual void mergeDeclaration(ConstraintVariable *, ProgramInfo &) = 0;
virtual void mergeDeclaration(ConstraintVariable *, ProgramInfo &,
std::string &ReasonFailed) = 0;

std::string getOriginalTy() const { return OriginalType; }
// Get the original type string that can be directly
Expand Down Expand Up @@ -343,7 +344,8 @@ class PointerVariableConstraint : public ConstraintVariable {
const CAtoms &getCvars() const { return vars; }

void brainTransplant(ConstraintVariable *From, ProgramInfo &I) override;
void mergeDeclaration(ConstraintVariable *From, ProgramInfo &I) override;
void mergeDeclaration(ConstraintVariable *From, ProgramInfo &I,
std::string &ReasonFailed) override;

static bool classof(const ConstraintVariable *S) {
return S->getKind() == PointerVariable;
Expand Down Expand Up @@ -456,7 +458,8 @@ class FunctionVariableConstraint : public ConstraintVariable {
}

void brainTransplant(ConstraintVariable *From, ProgramInfo &I) override;
void mergeDeclaration(ConstraintVariable *FromCV, ProgramInfo &I) override;
void mergeDeclaration(ConstraintVariable *FromCV, ProgramInfo &I,
std::string &ReasonFailed) override;

PVConstraint *getParamVar(unsigned i) const {
assert(i < ParamVars.size());
Expand Down
8 changes: 6 additions & 2 deletions clang/include/clang/3C/ProgramInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,18 @@ class ProgramInfo : public ProgramVariableAdder {
// Returns true if successful else false.
bool insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
const std::string &FuncName,
FVConstraint *ToIns);
FVConstraint *ToIns,
FunctionDecl *FD,
ASTContext *C);

// Inserts the given FVConstraint* set into the provided static map.
// Returns true if successful else false.
bool insertIntoStaticFunctionMap(StaticFunctionMapType &Map,
const std::string &FuncName,
const std::string &FileName,
FVConstraint *ToIns);
FVConstraint *ToIns,
FunctionDecl *FD,
ASTContext *C);


// Special-case handling for decl introductions. For the moment this covers:
Expand Down
28 changes: 22 additions & 6 deletions clang/lib/3C/ConstraintVariables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,12 +1589,17 @@ void PointerVariableConstraint::brainTransplant(ConstraintVariable *FromCV,
}

void PointerVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
ProgramInfo &Info) {
ProgramInfo &Info,
std::string &ReasonFailed) {
PVConstraint *From = dyn_cast<PVConstraint>(FromCV);
std::vector<Atom *> NewVatoms;
CAtoms CFrom = From->getCvars();
CAtoms::iterator I = vars.begin();
CAtoms::iterator J = CFrom.begin();
if (CFrom.size() != vars.size()) {
ReasonFailed = "conflicting types " + ReasonFailed;
return;
}
while (I != vars.end()) {
Atom *IAt = *I;
Atom *JAt = *J;
Expand Down Expand Up @@ -1626,7 +1631,7 @@ void PointerVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
ItypeStr = From->ItypeStr;
if (FV) {
assert(From->FV);
FV->mergeDeclaration(From->FV, Info);
FV->mergeDeclaration(From->FV, Info, ReasonFailed);
}
}

Expand Down Expand Up @@ -1678,14 +1683,19 @@ void FunctionVariableConstraint::brainTransplant(ConstraintVariable *FromCV,
}

void FunctionVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
ProgramInfo &I) {
ProgramInfo &I,
std::string &ReasonFailed) {
// `this`: is the declaration the tool saw first
// `FromCV`: is the declaration seen second, it cannot have defered constraints
FVConstraint *From = dyn_cast<FVConstraint>(FromCV);
assert(From != nullptr);
assert(From->getDeferredParams().size() == 0);
// Transplant returns.
ReturnVar->mergeDeclaration(From->ReturnVar, I);
// Our first sanity check is to ensure the function's returns type-check
std::string Base = ReasonFailed = "for return value";
ReturnVar->mergeDeclaration(From->ReturnVar, I, ReasonFailed);
if (ReasonFailed != Base) return;
ReasonFailed = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


if (From->numParams() == 0) {
// From is an untyped declaration, and adds no information
Expand All @@ -1695,11 +1705,17 @@ void FunctionVariableConstraint::mergeDeclaration(ConstraintVariable *FromCV,
From->brainTransplant(this, I);
} else {
// Standard merge
assert(this->numParams() == From->numParams());
if (this->numParams() != From->numParams()) {
ReasonFailed = "differing number of arguments";
return;
}
for (unsigned i = 0; i < From->numParams(); i++) {
auto *FromVar = From->getParamVar(i);
auto *Var = getParamVar(i);
Var->mergeDeclaration(FromVar, I);
Base = ReasonFailed = "for parameter " + std::to_string(i);
Var->mergeDeclaration(FromVar, I, ReasonFailed);
if (Base != ReasonFailed) return;
ReasonFailed = "";
}
}
}
Expand Down
48 changes: 39 additions & 9 deletions clang/lib/3C/ProgramInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,9 @@ void ProgramInfo::exitCompilationUnit() {
bool
ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
const std::string &FuncName,
FVConstraint *newC) {
FVConstraint *newC,
FunctionDecl *FD,
ASTContext *C) {
bool RetVal = false;
if (Map.find(FuncName) == Map.end()) {
Map[FuncName] = newC;
Expand All @@ -403,12 +405,38 @@ ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
newC->brainTransplant(oldC, *this);
Map[FuncName] = newC;
RetVal = true;
} else
} else {
// if the current FV constraint is not a definition?
// then merge.
oldC->mergeDeclaration(newC, *this);
} else if (newC->hasBody())
llvm::errs() << "Warning: duplicate definition " << FuncName << "\n";
std::string BaseFailed = "";
std::string ReasonFailed = "";
oldC->mergeDeclaration(newC, *this, ReasonFailed);
bool MergingFailed = BaseFailed != ReasonFailed;
if (MergingFailed) {
clang::DiagnosticsEngine &DE = C->getDiagnostics();
unsigned MergeFailID = DE.getCustomDiagID(DiagnosticsEngine::Fatal,
"merging failed for %q0 due to %1");
const auto Pointer = reinterpret_cast<intptr_t>(FD);
const auto Kind = clang::DiagnosticsEngine::ArgumentKind::ak_nameddecl;
auto DiagBuilder = DE.Report(FD->getLocation(), MergeFailID);
DiagBuilder.AddTaggedVal(Pointer, Kind);
DiagBuilder.AddString(ReasonFailed);
}
if(MergingFailed) {
// Kill the process and stop conversion
// Without this code here, 3C simply ignores this pair of functions
// and converts the rest of the files as it will (in semi-compliance
// with Mike's (2) listed on the original issue (#283)
exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
} else if (newC->hasBody()) {
clang::DiagnosticsEngine &DE = C->getDiagnostics();
unsigned DuplicateDefinitionsID = DE.getCustomDiagID(DiagnosticsEngine::Fatal,
"duplicate definition for function %0");
DE.Report(FD->getLocation(), DuplicateDefinitionsID).AddString(FuncName);
exit(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
// else oldc->hasBody() so we can ignore the newC prototype
}
return RetVal;
Expand All @@ -417,13 +445,15 @@ ProgramInfo::insertIntoExternalFunctionMap(ExternalFunctionMapType &Map,
bool ProgramInfo::insertIntoStaticFunctionMap (StaticFunctionMapType &Map,
const std::string &FuncName,
const std::string &FileName,
FVConstraint *ToIns) {
FVConstraint *ToIns,
FunctionDecl *FD,
ASTContext *C) {
bool RetVal = false;
if (Map.find(FileName) == Map.end()) {
Map[FileName][FuncName] = ToIns;
RetVal = true;
} else {
RetVal = insertIntoExternalFunctionMap(Map[FileName],FuncName,ToIns);
RetVal = insertIntoExternalFunctionMap(Map[FileName],FuncName,ToIns, FD, C);
}
return RetVal;
}
Expand All @@ -435,7 +465,7 @@ bool ProgramInfo::insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
if (FD->isGlobal()) {
// external method.
ret = insertIntoExternalFunctionMap(ExternalFunctionFVCons,
FuncName, FVCon);
FuncName, FVCon, FD, C);
bool isDef = FVCon->hasBody();
if (isDef) {
ExternFunctions[FuncName] = true;
Expand All @@ -448,7 +478,7 @@ bool ProgramInfo::insertNewFVConstraint(FunctionDecl *FD, FVConstraint *FVCon,
auto Psl = PersistentSourceLoc::mkPSL(FD, *C);
std::string FuncFileName = Psl.getFileName();
ret = insertIntoStaticFunctionMap(StaticFunctionFVCons, FuncName,
FuncFileName, FVCon);
FuncFileName, FVCon, FD, C);
}
return ret;
}
Expand Down
4 changes: 4 additions & 0 deletions clang/test/3C/difftypes_xfail1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//RUN: 3c -base-dir=%S -output-postfix=checked %s %S/difftypes_xfail2.c
// XFAIL: *

_Ptr<int> foo(int, char);
4 changes: 4 additions & 0 deletions clang/test/3C/difftypes_xfail2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//RUN: 3c -base-dir=%S -output-postfix=checked %s %S/difftypes_xfail1.c
// XFAIL: *

int * foo(int, char *);
8 changes: 8 additions & 0 deletions clang/test/3C/multidef_xfail1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//RUN: 3c -base-dir=%S -output-postfix=checked %s %S/multidef_xfail2.c
// XFAIL: *

_Ptr<int> foo(int x, char * y) {
x = x + 4;
int *z = &x;
return z;
}
8 changes: 8 additions & 0 deletions clang/test/3C/multidef_xfail2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//RUN: 3c -base-dir=%S -output-postfix=checked %s %S/multidef_xfail1.c
// XFAIL: *

int * foo(int x, _Ptr<char> y) {
x = x + 4;
int *z = &x;
return z;
}
24 changes: 24 additions & 0 deletions clang/test/3C/prototype_success1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//RUN: 3c -base-dir=%S -output-postfix=checked %s %S/prototype_success2.c
//RUN: FileCheck -match-full-lines --input-file %S/prototype_success1.checked.c %s
//RUN: %clang -c %S/prototype_success1.checked.c %S/prototype_success2.checked.c
//RUN: rm %S/prototype_success1.checked.c %S/prototype_success2.checked.c

/*Note: this file is part of a multi-file regression test in tandem with prototype_success2.c*/

/*prototypes that type-check with each other are fine*/
int * foo(_Ptr<int>, char);

/*a prototype definition combo that type-checks is also fine*/
int *bar(int *x, float *y) {
return x;
}

/*a C-style prototype combined with an enumerated prototype is also fine*/
int *baz(int);

/*another consideration is that if two things are different types but have the same general pointer structure, we're OK with it!*/
int *yoo(int *x, char y, float **z);

void trivial_conversion(int *x) {
//CHECK: void trivial_conversion(_Ptr<int> x) {
}
18 changes: 18 additions & 0 deletions clang/test/3C/prototype_success2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//RUN: 3c -base-dir=%S -output-postfix=checked2 %s %S/prototype_success1.c
//RUN: FileCheck -match-full-lines --input-file %S/prototype_success2.checked2.c %s
//RUN: rm %S/prototype_success1.checked2.c %S/prototype_success2.checked2.c

/*Note: this file is part of a multi-file regression test in tandem with prototype_success1.c
For comments about the different functions in this file, please refer to prototype_success1.c*/

_Ptr<int> foo(int *, char);

int *bar(int *, float *);

int *baz();

_Ptr<int> yoo(char *x, float y, int **z);

void trivial_conversion2(int *x) {
//CHECK: void trivial_conversion2(_Ptr<int> x) {
}