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 jemalloc compatibility with LLVM libunwind #37078

Merged
merged 2 commits into from
May 23, 2022

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented May 10, 2022

jemalloc provides support for two different libunwind flavors: the original HP libunwind and the one coming with gcc / g++ / libstdc++.

The latter is identified by JEMALLOC_PROF_LIBGCC and uses _Unwind_Backtrace method instead of unw_backtrace.
At the time ClickHouse uses LLVM libunwind which follows libgcc's way of backtracing.

ClickHouse has to provide unw_backtrace method by the means of commit 8e2b31e.

While this PR does not allow complete removal of the patch (as ClickHouse itself uses unw_backtrace directly), it definitely sorts some things out.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

jemalloc provides support for two different libunwind flavors: the original HP libunwind and the one coming with gcc / g++ / libstdc++.

The latter is identified by `JEMALLOC_PROF_LIBGCC` and provides `_Unwind_Backtrace` method instead of `unw_backtrace`.
At the time ClickHouse uses LLVM libunwind which follows libgcc's way of backtracing.

ClickHouse has to provide `unw_backtrace` method by the means of [commit 8e2b31e](ClickHouse/libunwind@8e2b31e).

While this PR does not allow complete remove of the patch (as ClickHouse itself uses unw_backtrace directly), it definitely sorts the things out.
@robot-ch-test-poll2 robot-ch-test-poll2 added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels May 10, 2022
@nikitamikhaylov nikitamikhaylov added the can be tested Allows running workflows for external contributors label May 10, 2022
@alexey-milovidov alexey-milovidov self-assigned this May 10, 2022
@georgthegreat
Copy link
Contributor Author

@alexey-milovidov, @nikitamikhaylov, could you merge this?

@alexey-milovidov
Copy link
Member

Functional test has failed: https://s3.amazonaws.com/clickhouse-test-reports/37078/6d28b226878f935505edef2606c860da3b58bc9b/stateless_tests__thread__actions__[3/3].html

Obviously, it is not related to the changes in this PR but I will use this fact to highlight the principle that pull requests should be 100% green, regardless. We need to investigate and fix the test and only then merge this PR.

@alexey-milovidov
Copy link
Member

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented May 23, 2022

update

✅ Branch has been successfully updated

@alexey-milovidov alexey-milovidov merged commit cf8eb41 into ClickHouse:master May 23, 2022
@alexey-milovidov
Copy link
Member

I merged prematurely, but the code is non-obvious, it needs a comment.

It's literally impossible to understand, why

if (USE_UNWIND)
    target_compile_definitions (_jemalloc PRIVATE -DJEMALLOC_PROF_LIBGCC=1)

when libunwind is enabled we use "libgcc" for profiling in jemalloc.
It will introduce "WTF" when anyone will read the code.

Need to copy-paste your description from the pull request to the code.

alexey-milovidov added a commit that referenced this pull request May 24, 2022
@georgthegreat georgthegreat deleted the patch-2 branch May 24, 2022 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants