Page MenuHomePhabricator

[scudo][standalone] Introduce the C & C++ wrappers [fixed]
ClosedPublic

Authored by cryptoad on Jun 26 2019, 11:10 AM.

Details

Summary

This is a redo of D63612.

Two problems came up on some bots:

  • __builtin_umull_overflow was not declared. This is likely due to an older clang or gcc, so add a guard with __has_builtin and fallback to a division in the event the builtin doesn't exist;
  • contradicting definition for malloc, etc. This is AFAIU due to the fact that we ended up transitively including stdlib.h in the .inc due to it being the flags parser header: so move the include to the cc instead.

This should fix the issues, but since those didn't come up in my local
tests it's mostly guesswork.

Rest is the same!

Diff Detail

Repository
rL LLVM

Event Timeline

cryptoad created this revision.Jun 26 2019, 11:10 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 26 2019, 11:11 AM
Herald added subscribers: Restricted Project, jfb, delcypher and 2 others. · View Herald Transcript
This revision is now accepted and ready to land.Jun 26 2019, 11:46 AM
dyung accepted this revision.Jun 26 2019, 3:14 PM

I applied your patch and built it on our internal build bot that failed and these changes worked, so LGTM as well!

Thank you Douglas!

hans accepted this revision.Jun 27 2019, 12:17 AM

Thank you Douglas!

Seems to work for us (Chromium) too. (I didn't actually look at the code, I just applied it in my build.)

cryptoad updated this revision to Diff 206862.Jun 27 2019, 7:22 AM

clang-format'd.

This revision was automatically updated to reflect the committed changes.

I am getting failures when trying to link these tests, due to missing symbols, e.g.:

cd projects/compiler-rt/lib/scudo/standalone/tests && ./bin/clang ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o ScudoUnitTestsObjects.scudo_unit_test_main.cc.i386.o ScudoUnitTestsObjects.gtest-all.cc.i386.o lib/libRTScudoCUnitTest.i386.a -o projects/compiler-rt/lib/scudo/standalone/tests/./ScudoCUnitTest-i386-Test -Wl,-allow-shlib-undefined -lstdc++ -pthread -m32

lib/libRTScudoCUnitTest.i386.a(wrappers_c.cc.o): In function `scudo::atomic_u64::Type scudo::atomic_load<scudo::atomic_u64>(scudo::atomic_u64 const volatile*, scudo::memory_order)':
projects/compiler-rt/lib/scudo/standalone/atomic_helpers.h:66: undefined reference to `__atomic_load_8'
...

and other missing symbols.

It passes if I link manually and add in -latomic. Should that be added to the link flags somewhere? How does this work on the bots or elsewhere?

rnk added a subscriber: rnk.Jul 1 2019, 4:24 PM

We're getting link errors about operator delete in 32-bit builds:

[197/771] Generating ScudoCUnitTest-i386-Test
FAILED: projects/compiler-rt/lib/scudo/standalone/tests/ScudoCUnitTest-i386-Test 
cd /b/swarming/w/ir/cache/builder/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/scudo/standalone/tests && /b/swarming/w/ir/cache/builder/src/third_party/llvm-bootstrap/./bin/clang ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o ScudoUnitTestsObjects.scudo_unit_test_main.cc.i386.o ScudoUnitTestsObjects.gtest-all.cc.i386.o /b/swarming/w/ir/cache/builder/src/third_party/llvm-bootstrap/lib/libRTScudoCUnitTest.i386.a -o /b/swarming/w/ir/cache/builder/src/third_party/llvm-bootstrap/projects/compiler-rt/lib/scudo/standalone/tests/./ScudoCUnitTest-i386-Test -fno-pie -Wl,-allow-shlib-undefined -lstdc++ -pthread -m32
ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o: In function `__cxx_global_var_init.1':
wrappers_c_test.cc:(.text.startup+0x17a): undefined reference to `operator delete(void*, unsigned int)'
ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o: In function `__cxx_global_var_init.18':
wrappers_c_test.cc:(.text.startup+0x2ea): undefined reference to `operator delete(void*, unsigned int)'
ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o: In function `__cxx_global_var_init.24':
wrappers_c_test.cc:(.text.startup+0x45a): undefined reference to `operator delete(void*, unsigned int)'
ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o: In function `__cxx_global_var_init.33':
wrappers_c_test.cc:(.text.startup+0x5ca): undefined reference to `operator delete(void*, unsigned int)'
ScudoUnitTestsObjects.wrappers_c_test.cc.i386.o: In function `__cxx_global_var_init.36':
wrappers_c_test.cc:(.text.startup+0x73a): undefined reference to `operator delete(void*, unsigned int)'

It would be enough for us to exclude these tests from check-all, so rather than reverting, I might do that.

Tentative fix for this is in https://reviews.llvm.org/D64086, if you could please let me know if that works for you.

In D63831#1565702, @rnk wrote:

We're getting link errors about operator delete in 32-bit builds:

Regarding Teresa's issue, it seems to boil down to the version of clang, and not inlining the __atomic_load_8 & co.
Looking at godbolt, it doesn't seem to happen until clang 6, while gcc did it since a while.
After attempting to conditionally add -latomic, and failing, I am resorting to adding -latomic unconditionally to the tests link flags, unless someone has a better idea.

It passes if I link manually and add in -latomic. Should that be added to the link flags somewhere? How does this work on the bots or elsewhere?