Skip to content
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

Jump to bottom

Fix 3845 nullptr cpp #3958

Merged
merged 2 commits into from
Nov 14, 2022
Merged

Fix 3845 nullptr cpp #3958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbd428f
Fixes #3845; C++ target was deref'ing nullptr when examining array of…
parrt Nov 14, 2022
fa7aad7
Drop a test that exposes another bug.
parrt Nov 14, 2022
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
3 changes: 0 additions & 3 deletions ...ources/org/antlr/v4/test/runtime/descriptors/ParserExec/ListLabelsOnRuleRefStartOfAlt.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,3 @@ expression

[input]
a and b

[skip]
Cpp
4 changes: 4 additions & 0 deletions ...urces/org/antlr/v4/test/runtime/descriptors/Performance/DropLoopEntryBranchInLRRule_4.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
for https://github.com/antlr/antlr4/issues/1398.
Seeing through a large expression takes 5 _minutes_ on
my fast box to complete. After fix, it's instantaneous.
Was working for C++ but I think it was not parsing correctly (Nov 2022)
So I'll make it skip for now and will add bug. I believe it is not
merging arrays properly.

[type]
Parser
Expand Down Expand Up @@ -48,6 +51,7 @@ between X1 and X2 or between X3 and X4
;

[skip]
Cpp
Go
Python2
Python3
Expand Down
21 changes: 16 additions & 5 deletions runtime/Cpp/runtime/src/atn/ArrayPredictionContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ namespace {
}

bool predictionContextEqual(const Ref<const PredictionContext> &lhs, const Ref<const PredictionContext> &rhs) {
// parent PredictionContext pointers can be null during full context mode and
// the ctxs are in an ArrayPredictionContext. If both are null, return true
// if just one is null, return false. If both are non-null, do comparison.
if (lhs == nullptr && rhs == nullptr) return true;
if (lhs != nullptr || rhs != nullptr) return false;
return *lhs == *rhs;
Comment on lines +28 to 30
Copy link
Member

@KvanTTT KvanTTT Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It looks incorrect. Last line is never executed since it return false if any or both arguments are not null. Also, it's not optimal. I suggest fixing in the following way:

Suggested change
if (lhs == nullptr && rhs == nullptr) return true;
if (lhs != nullptr || rhs != nullptr) return false;
return *lhs == *rhs;
if (lhs == nullptr)
return rhs == nullptr;
else
return rhs != nullptr;
return *lhs == *rhs;

Copy link
Member Author

Choose a reason for hiding this comment

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

dang! you're right. I tried to get too clever. grrr...will fix. thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

is your line 32 dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe this?

if ( lhs == nullptr ) return rhs == nullptr;
if ( rhs == nullptr ) return false; // lhs!=null and rhs==null
return *lhs == *rhs;                // both nonnull

Copy link
Member

Choose a reason for hiding this comment

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

Not dead, it's comparison by dereference (if I remember and understand C++ correctly).

Copy link
Member Author

Choose a reason for hiding this comment

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

meaning your line 32 is never reached.

Copy link
Member

@KvanTTT KvanTTT Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, my first suggestion is also incorrect. The fixed version:

    if (lhs == nullptr)
        return rhs == nullptr;
    else if (rhs == nullptr)
        return false;
    return *lhs == *rhs;

Copy link
Member Author

Choose a reason for hiding this comment

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

yep trying similar now. seems to work. this one still fails though, but this time with a SEGV #3959 rather than infinite loop.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's correct. Didn't get update in time.

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed the fix. thanks for finding! I updated trace diff for failing test: #3959

}

Expand Down Expand Up @@ -75,11 +80,17 @@ bool ArrayPredictionContext::equals(const PredictionContext &other) const {
return false;
}
const auto &array = downCast<const ArrayPredictionContext&>(other);
return returnStates.size() == array.returnStates.size() &&
parents.size() == array.parents.size() &&
cachedHashCodeEqual(cachedHashCode(), array.cachedHashCode()) &&
std::memcmp(returnStates.data(), array.returnStates.data(), returnStates.size() * sizeof(decltype(returnStates)::value_type)) == 0 &&
std::equal(parents.begin(), parents.end(), array.parents.begin(), predictionContextEqual);
const bool sameSize = returnStates.size() == array.returnStates.size() &&
parents.size() == array.parents.size();
const bool sameHash = cachedHashCodeEqual(cachedHashCode(), array.cachedHashCode());
const size_t stateSizeBytes = sizeof(decltype(returnStates)::value_type);
const bool returnStateArraysEqual =
std::memcmp(returnStates.data(), array.returnStates.data(),
returnStates.size() * stateSizeBytes) == 0;
// stack of contexts is the same
const bool parentCtxEqual =
std::equal(parents.begin(), parents.end(), array.parents.begin(), predictionContextEqual);
return sameSize && sameHash && returnStateArraysEqual && parentCtxEqual;
}

std::string ArrayPredictionContext::toString() const {
Expand Down