Fix race condition in clang_macro_fallback

[HKalbasi]

Previously, bindgen by default used the same directory of the headers as the directory for temporary files of the clang_macro_fallback feature. Aside from its other problems like making temporary noises in git or failing to remove these files on failing or aborted builds, this caused a race condition in build of retina that I spend two DAYS to figure it out.

The problem is, if you run bindgen on the same set of files multiple times simultaneously (which happened in retina since a crate was included two times in the crate graph due different features, and it ran bindgen in its build script) one of them can remove clang_macro_fallback files of the other one and break it.

I fixed it by moving the files generated by clang_macro_fallback in a new temp directory each time bindgen is invoked.

[ojeda]

Cc @jbaublitz since he is probably interested.

[jbaublitz]

@HKalbasi You may want to base this PR on #3072 as that is the PR I put up to address some problems that @ojeda bumped into while using the clang fallback code.

[HKalbasi]

I will do the rebase after your PR is merged so I don't have to rebase it multiple times in case your PR needs further changes, and to keep the diff clean if you or anyone else want to review it in the meantime.

[jbaublitz]

@HKalbasi Okay, it looks like the PR got merged.

[HKalbasi]

@jbaublitz What is your opinion on this PR?

[jbaublitz]

@HKalbasi I'm not the maintainer so I'm not whether they're expecting concurrent builds or not. I'd assume they'd probably want to support that use case but you'd have to ask one of the maintainers.

As for my thoughts on this PR, I think this is one way to solve this problem and I'm not entirely against it but I noticed in your original statement of the issue that you mentioned that it makes temporary noise in git. This is the rationale for clang_macro_fallback_build_dir. Theoretically you should be able to not only point to a separate directory so that the temporary files don't show up anywhere in the git tree but I think on both the Rust and the bash side, it is possible to pass a separate temporary directory to each invocation of bindgen. This is an extra step that you address in this PR automatically but I'd want to hear @ojeda's opinion on whether taking control away from the user for where their temporary files are created is desirable. This may not play nicely with kernel stuff, I'm not sure.

[HKalbasi]

The main problem this PR addresses is that if you run two instance of bindgen on the same set of files (which is easy with build scripts) you will get a hard to debug race condition on build with the default settings, and also with constant clang_macro_fallback_build_dir. If the kernel can't use the /tmp directory and needs clang_macro_fallback_build_dir I can add it again with a warn comment about this race condition.

[jbaublitz]

@ojeda What do you think here? Is relying on /tmp when this will likely eventually be used for kernel builds doable? Based on all of the wide variety of environments in which kernel builds take place, I could imagine it not being available in some of them.

I can think of a few additional options we could also explore. For one, we could create a new randomized alphanumeric subdirectory under clang_macro_fallback_build_dir that would be distinct per invocation of bindgen. Namespacing the temporary files would likely solve this issue without needing to rely on /tmp.

[ojeda]

In general, I would say not allowing to configure the folder for temporaries is not a good idea.

In the kernel, we create some temporary directories inside the build folder (which is configurable) rather than /tmp. On the other hand, some tools do attempt to create things in /tmp, and some even fail they are not allowed to, but as long as one configures $TMPDIR, the build seems to be intended to work even if /tmp is not accessible (e.g. I see a script allowing to customize it with $TMPDIR and I also tested a minimal kernel build without access to /tmp just to double-check).

So I would say allowing users to override the /tmp would be needed for the kernel or making bindgen respect e.g. $TMPDIR like GCC and Clang do (does tempfile support so already?).

See:

https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html#index-TMPDIR
https://clang.llvm.org/docs/CommandGuide/clang.html#envvar-TMPDIR-TEMP-TMP

[HKalbasi]

does tempfile support so already?

tempfile uses std::env::temp_dir for finding the temp directory location, so indeed it does respect $TMPDIR.

I would say not allowing to configure the folder for temporaries is not a good idea.

Is $TMPDIR enough as a configuration, or do you prefer an explicit bindgen configuration for that?

[ojeda]

If $TMPDIR works, then I think that should be good enough.