Page MenuHomePhabricator

[asan] Rename the ABI versioning symbol to '__asan_version_mismatch_check' instead abusing '__asan_init'
ClosedPublic

Authored by kubamracek on Jul 7 2015, 10:38 AM.

Details

Summary

Hi everyone,

I'd like to propose a change in the way we version the ASan libraries (and other sanitizers as well), and I'd like to get feedback on this proposal. We currently have the "ABI version" number embedded into the ASan initializer symbol name (__asan_init_v5), which is occasionally bumped when there is a significant ABI-breaking change. My understanding is that this number doesn't guarantee compatibility. It indicates that the ABI changed, but there might have been smaller incompatible changes when the version is unchanged. This can easily cause issues especially when using dynamic libraries (.dylib, .so), because they are (and need to be) distributed with applications and libraries, so users can easily end up using a stale version of the ASan library.

I'd like to include the LLVM release number into that symbol name so users are actually forced to use the library that was distributed with the compiler they used. We should also provide a way for vendors who make their own releases to change this version to indicate that it's a different build than the open-source one. Local (development) builds should also have a different version. This could reuse a version-string schema already used by Clang.

Secondly, I'd like to make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (following by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Attached is an incomplete (work in progress, llvm backend part missing) patch which outlines how this could be done.

Any feedback is welcome!

Kuba

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 29191.Jul 7 2015, 10:38 AM
kubamracek retitled this revision from to [asan] Change ABI versioning to include release numbers.
kubamracek updated this object.
kubamracek added reviewers: glider, samsonov, kcc.
kubamracek added subscribers: llvm-commits, glider, samsonov and 2 others.
kcc edited edge metadata.Jul 7 2015, 10:52 AM

I don't like it. While it may solve your problem, it will create new problems for us.
asan run-time is build by several different build systems and your patch modifies one of them (llvm cmake),
but it requires to change all others (gcc's makefiles and our internal build system at least, probably more).

I would prefer a solution that does not change the build files, or a solution that changes *only* build files (thus being llvm-specific)

Can I also have your opinion on the renaming of the symbol, e.g.:

... make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Thanks!

kcc added a comment.Jul 9 2015, 10:56 AM
In D11004#201766, @kubabrecka wrote:

Can I also have your opinion on the renaming of the symbol, e.g.:

... make the error message about a version mismatch clearer by changing the name of the undefined symbol to be something like __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol.

Yes, this part clearly makes sense.

Thanks!

kubamracek updated this revision to Diff 30261.Jul 21 2015, 9:06 AM
kubamracek retitled this revision from [asan] Change ABI versioning to include release numbers to [asan] Rename the ABI versioning symbol to '__asan_version_mismatch_check' instead abusing '__asan_init'.
kubamracek edited edge metadata.

Hi, I've removed the build system changes, and now this patch is only about renaming the symbol.

We currently version __asan_init and when the ABI version doesn't match, the linker gives a undefined reference to '__asan_init_v5' message. From this, it might not be obvious that it's actually a version mismatch error. This patch makes the error message much clearer by changing the name of the undefined symbol to be __asan_version_mismatch_check_xxx (followed by the version string). We obviously don't want the initializer to be named like that, so it's a separate symbol that is used only for the purpose of version checking.

kcc accepted this revision.Jul 21 2015, 5:45 PM
kcc edited edge metadata.

LGTM
Please watch the bots once you commit, this is a potentially disrupting change.

This revision is now accepted and ready to land.Jul 21 2015, 5:45 PM
kubamracek closed this revision.Jul 23 2015, 3:56 AM

Landed in r243003 (llvm part) and r243004 (compiler-rt part). Thanks for the review!

Windows bot was failing some tests, so I had to add the new interface function into asan_win_dll_thunk.cc in r243005 and r243007.