Sniff for libclang-version-correct headers #1325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🙈 |
|
I got cha! #1326 You can probably close this. |
|
Hmm, yeah I won't have time to investigate a "proper" fix right now. Thanks! |
|
Actually, it turns out I don't feel I have a choice! |
workingjubilee
force-pushed
the
fix-pg16-clang-problems
branch
2 times, most recently
from
561b267
to
cbafd30
Compare
|
Okay, so the answer is to override the include path so that it points invariably at the same root. This is the map of our includes: |
|
not sure how this brings in llvm-14 headers
|
|
How does PGXS handle the include paths? Do C extensions suffer this problem too? |
|
I have no idea and no idea where I'd even start looking, and I don't think it matters, because this is about system configuration, clang's search paths, and whether those aspects are controlled. |
|
Hell yeah, all tests cleared. |
|
Now to make it an actually modular solution instead of hardcoded pile of mess. ⚰️ |
|
It looks like https://github.com/KyleMayes/clang-sys/blob/master/src/support.rs#L60 (used by bindgen) is finding the wrong clang, calling it to determine the include paths. For system header paths this is fine, but for builtin headers (like I don't know the solution here, but having mismatches like this could potentially expand beyond failing to find this header, and could cause us to generate bindings that are subtly wrong, depending on what PG does with |
|
Where in bindgen actually hits that codepath? I've been tracing, looking for that. |
workingjubilee
force-pushed
the
fix-pg16-clang-problems
branch
from
f799a99
to
660f2c3
Compare
|
This PR has been reduced to the minimal viable solution: Avoid using |
|
I have also opened |
|
This minimal fix does not make this work on certain Linux distros we might be reasonably expected to support. I am not just talking about CentOS 7. If it was just CentOS 7, I'd feel okay telling everyone who wants that support to go to... more rapidly update to an in-support distro. |
workingjubilee
force-pushed
the
fix-pg16-clang-problems
branch
from
ebaec10
to
5581b63
Compare
|
I win. |
Filter when filtering redux
Ship the miserable hack.
This reverts commit 641eb11.
workingjubilee
force-pushed
the
fix-pg16-clang-problems
branch
from
044c1fd
to
9d89601
Compare
workingjubilee
changed the title
|
Feel free to tell me how awful and untenable my logic is on macOS or whatever. |
workingjubilee
changed the title
|
also even if it's somewhat bogus on another OS, I really do intend to send in the patch to Postgres, so that Postgres 16.2 or whatever doesn't have this problem, I just want to unblock pgrx dev. |
eeeebbbbrrrr
approved these changes
Nov 16, 2023
|
I think this will be wrong for macOS if it does anything, but need to look more closely (I'll do it before EOD) |
|
Mmkay! I'm happy to basically stub this out for macOS. |
|
Also I submitted https://www.postgresql.org/message-id/CAPNHn3oKJJxMsYq%2BqLYzVJOFrUcOr4OF1EC-KtFT-qh8nOOOtQ%40mail.gmail.com so hopefully this will simply stop being a problem for Postgres |
workingjubilee
commented
Nov 16, 2023
Comment on lines
+16
to
+47
| pub(crate) fn detect_include_paths_for( | ||
| preferred_clang: Option<&std::path::Path>, | ||
| ) -> (bool, Vec<PathBuf>) { | ||
| if target_env_tracked("PGRX_BINDGEN_NO_DETECT_INCLUDES").is_some() { | ||
| return (false, vec![]); | ||
| } | ||
|
|
||
| // By asking bindgen for the version, we force it to pull an appropriate libclang, | ||
| // allowing users to override it however they would usually override bindgen. | ||
| let clang_major = match bindgen::clang_version() { | ||
| ClangVersion { parsed: Some((major, _)), full } => { | ||
| eprintln!("Bindgen found {full}"); | ||
| major | ||
| } | ||
| ClangVersion { full, .. } => { | ||
| // If bindgen doesn't know what version it has, bail and hope for the best. | ||
| eprintln!("Bindgen failed to parse clang version: {full}"); | ||
| return (true, vec![]); | ||
| } | ||
| }; | ||
|
|
||
| // If Postgres is configured --with-llvm, then it may have recorded a CLANG to use | ||
| // Ask if there's a clang at the path that Postgres would use for JIT purposes. | ||
| // Unfortunately, the responses from clang-sys include clangs from far-off paths, | ||
| // so we can only use clangs that match bindgen's libclang major version. | ||
| if let Some(ClangSys { path, version: Some(v), c_search_paths, .. }) = | ||
| ClangSys::find(preferred_clang, &[]) | ||
| { | ||
| if Some(&*path) == preferred_clang && v.Major as u32 == clang_major { | ||
| return (false, c_search_paths.unwrap_or_default()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the part that I feel deeply confident is basically guaranteed-correct for macOS, but I must admit I might inadvertantly be counting on "clang vs. appleclang doesn't matter for the headers we need".
Comment on lines
+49
to
+67
| // Oh no, still here? | ||
| // Let's go behind bindgen's back to get libclang's path | ||
| let libclang_path = | ||
| clang_sys::get_library().expect("libclang should have been loaded?").path().to_owned(); | ||
| eprintln!("found libclang at {}", libclang_path.display()); | ||
| // libclang will probably be in a dynamic library directory, | ||
| // which means it will probably be adjacent to its headers, e.g. | ||
| // - "/usr/lib/libclang-${CLANG_MAJOR}.so.${CLANG_MAJOR}.${CLANG_MINOR}" | ||
| // - "/usr/lib/clang/${CLANG_MAJOR}/include" | ||
| let clang_major_fmt = clang_major.to_string(); | ||
| let mut paths = vec![]; | ||
| // by adjacent, that does not mean it is always immediately so, e.g. | ||
| // - "/usr/lib/x86_64-linux-gnu/libclang-${CLANG_MAJOR}.so.${CLANG_MAJOR}.${CLANG_MINOR}.${CLANG_SUBMINOR}" | ||
| // - "/usr/lib/clang/${CLANG_MAJOR}/include" | ||
| // or | ||
| // - "/usr/lib64/libclang-${CLANG_MAJOR}.so.${CLANG_MAJOR}.${CLANG_MINOR}.${CLANG_SUBMINOR}" | ||
| // - "/usr/lib/clang/${CLANG_MAJOR}/include" | ||
| // so, crawl back up the ancestral tree | ||
| for ancestor in libclang_path.ancestors() { |
There was a problem hiding this comment.
This is the part where the secret ingredient is Crime. The idea is that this crime starts its journey relative to the libclang.so dir so that we ideally find the right headers to pull even if we're starting from an unusual place.
Comment on lines
+73
to
+91
| // On Unix-y systems this will be like "/usr/lib/clang/$CLANG_MAJOR/include" | ||
| // so don't even descend if the directory doesn't have one of those parts | ||
| .filter_entry(|entry| { | ||
| !is_hidden(entry) && { | ||
| entry_contains(entry, "clang") | ||
| || entry_contains(entry, "include") | ||
| || entry_contains(entry, &*clang_major_fmt) | ||
| // we always want to descend from a lib dir, but only one step | ||
| // as we don't really want to search all of /usr/lib's subdirs | ||
| || os_str_contains(entry.file_name(), "lib") | ||
| } | ||
| }) | ||
| .filter_map(|e| e.ok()) // be discreet | ||
| // We now need something that looks like it actually satisfies all our constraints | ||
| .filter(|entry| { | ||
| entry_contains(entry, &*clang_major_fmt) | ||
| && entry_contains(entry, "clang") | ||
| && entry_contains(entry, "include") | ||
| }) |
There was a problem hiding this comment.
The fast-and-loose way this may seem to be written is to account for possible differences like
- /usr/lib/clang-15/include
- /usr/lib/clang/15/include
It may seem unnecessary, but we don't necessarily want to have a hard dependency on there being a clang executable. This is because it is possible for libclang to be packaged without its executable and this is a configuration that is actually shipped by real distros.
You may think that seems... bad... but it's less goofy than the configurations that package libclang separately from its headers!
Comment on lines
+92
to
+104
| // we need to pull the actual directories that include the SIMD headers | ||
| .filter(|entry| { | ||
| os_str_contains(entry.file_name(), "emmintrin.h") | ||
| || os_str_contains(entry.file_name(), "arm_neon.h") | ||
| }) | ||
| .filter_map(|entry| { | ||
| let mut pbuf = entry.into_path(); | ||
| if pbuf.pop() && pbuf.is_dir() && os_str_contains(&*pbuf.file_name()?, "include") { | ||
| Some(pbuf) | ||
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
This is where I'm hoping this is basically enough validation that we eliminate any real risk of pulling the wrong headers. This also prevents pulling entries like:
- /usr/lib/clang/17/include/ppc_wrappers/emmintrin.h
fwiw I did run the nightly "all distros" workflow on this, but as the dispatch workflow currently works I can't prove that's due to anything, really. |
thomcc
approved these changes
Nov 16, 2023
workingjubilee
added a commit
that referenced
this pull request
Nov 27, 2023
Bindgen's strategy of autodetecting headers attempts to use a hacky route used by clang-sys, however, it doesn't take into account that the strategy of clang-sys uses `$CLANG_PATH` instead of `$LIBCLANG_PATH`, and then falls back to searching the entire directory structure. This means it's fairly easy for it to pull the headers for a different version, trying to compile them with the wrong clang! This replaces their series of unfortunate hacks with our own, using the collected information from pg_config, clang-sys, and rust-bindgen to try to locate the correct compiler headers. If it can't find anything, it falls back to doing what currently happens.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bindgen's strategy of autodetecting headers attempts to use a hacky route used by clang-sys, however, it doesn't take into account that the strategy of clang-sys uses
$CLANG_PATHinstead of$LIBCLANG_PATH, and then falls back to searching the entire directory structure. This means it's fairly easy for it to pull the headers for a different version, trying to compile them with the wrong clang!This adopts a mesh of approaches, using the collected information from pg_config, clang-sys, and rust-bindgen to try to locate the correct compiler headers. If it can't find anything, it falls back to doing what currently happens.