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.

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

fix stack overflow issue when using initializer_list #9367

Merged
merged 1 commit into from Feb 26, 2020
Merged

fix stack overflow issue when using initializer_list #9367

merged 1 commit into from Feb 26, 2020

Conversation

ghost
Copy link

@ghost ghost commented Feb 25, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix unit test collapsing_sorted_stream.

Detailed description / Documentation draft:

Fix stack overflow issue in copy on write pointer when using initializer_list
The create function inside COWHelper calls itself when called with initializer_list

@KochetovNicolai
Copy link
Member

It is not clear for me what problem was solved. Could you please explain it a little bit or add a test?

@ghost
Copy link
Author

ghost commented Feb 26, 2020

hello @KochetovNicolai, when running dbms/src/DataStreams/tests/collapsing_sorted_stream.cpp, i've an exit code 11 and when debugging with lldb, i found the stack is full of calls on static MutablePtr create(std::initializer_list<T> && arg) { return create(std::forward<std::initializer_list<T>>(arg)); }

It seems that this method is calling itself recursively.

I'm running the program on MacOS 10.14.6 with clang 8.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Feb 26, 2020

I have also faced this issue. It cannot happen in production, because this constructor is unused (it only happens in separate test programs). I tried to fix it some time ago but faced some difficulties. Need to review the fix.

@ghost
Copy link
Author

ghost commented Feb 26, 2020

Thanks for your feedback, @alexey-milovidov. Feel free to comment on the code. The fix is done together with @LeoErcolanelli who contributed to some features previsouly.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

The fix looks Ok (and previous code was looking like an obvious infinite recursion).

@@ -30,13 +30,13 @@ try
ColumnWithTypeAndName column1;
column1.name = "Sign";
column1.type = std::make_shared<DataTypeInt8>();
column1.column = ColumnInt8::create({1, -1});
column1.column = ColumnInt8::create({static_cast<int8_t>(1), static_cast<int8_t>(-1)});
Copy link
Member

Choose a reason for hiding this comment

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

Why don't make the type of initializer list generic in ColumnVector and PODArray constructors? Let me try to do it in subsequent PR...

@alexey-milovidov alexey-milovidov added pr-build Pull request with build/testing/packaging improvement no-docs-needed labels Feb 26, 2020
@alexey-milovidov alexey-milovidov merged commit ab0bb7a into ClickHouse:master Feb 26, 2020
@ghost
Copy link
Author

ghost commented Feb 27, 2020

@alexey-milovidov is it normal that the CI is not completely green? Do tests fail randomly?

@alexey-milovidov
Copy link
Member

It isn't. Fails are not random.

alexey-milovidov added a commit that referenced this pull request Feb 28, 2020
* Generic constructor of ColumnVector from initializer list #9367

* Fixed build

* Fixed error
alesapin pushed a commit that referenced this pull request Apr 13, 2020
fix stack overflow issue when using initializer_list

(cherry picked from commit ab0bb7a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-build Pull request with build/testing/packaging improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants