Add support for compilation-hints proposal #2627
Conversation
…tr_freq` and `metadata.code.call_targets` annotations and sections
5aa5685 to
d43cd08
Compare
| name(name), | ||
| type(Type::InstructionFrequency) { | ||
| new (&hint.instruction_frequency) | ||
| InstructionFrequency(instruction_frequency); |
There was a problem hiding this comment.
Can you just do hint.instruction_frequency = instruction_frequency here instead of the placement new?
There was a problem hiding this comment.
Since hint.instruction_frequency is not a valid C++ object, we're not allowed to use its assignment copy operator. In practice, using it should work as expected, but it is still undefined behavior nevertheless (afaik). That's why I opted for just writing over it, which should definitely be safe.
There was a problem hiding this comment.
Can you explain why more? Why can't we using the assignment operator here? Surely the LHS doesn't matter here since its being clobbered by the RHS.
Does the compiler complain if you do hint.instruction_frequency = instruction_frequency?
There was a problem hiding this comment.
While the compiler does not catch the undefined behavior, ASAN does:
+AddressSanitizer:DEADLYSIGNAL
+=================================================================
+==720746==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x55d1e4e29fd8 bp 0x000000000002 sp 0x7fffd147b960 T0)
+==720746==The signal is caused by a READ memory access.
+==720746==Hint: this fault was caused by a dereference of a high value address (see register values below). Disassemble the provided pc to learn which register was used.
+ #0 0x55d1e4e29fd8 in atomic_compare_exchange_strong<__sanitizer::atomic_uint8_t> /home/abuild/rpmbuild/BUILD/llvm20-20.1.8-build/llvm-20.1.8.src/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_atomic_clang.h:84:10
+ #1 0x55d1e4e29fd8 in AtomicallySetQuarantineFlagIfAllocated /home/abuild/rpmbuild/BUILD/llvm20-20.1.8-build/llvm-20.1.8.src/projects/compiler-rt/lib/asan/asan_allocator.cpp:669:10
+ #2 0x55d1e4e29fd8 in __asan::Allocator::Deallocate(void*, unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType) /home/abuild/rpmbuild/BUILD/llvm20-20.1.8-build/llvm-20.1.8.src/projects/compiler-rt/lib/asan/asan_allocator.cpp:733:10
+ #3 0x55d1e4f12b9b in operator delete(void*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm20-20.1.8-build/llvm-20.1.8.src/projects/compiler-rt/lib/asan/asan_new_delete.cpp:155:3
+ #4 0x55d1e4f1c4a4 in std::__new_allocator<unsigned char>::deallocate(unsigned char*, unsigned long) /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/new_allocator.h:172:2
+ #5 0x55d1e4f1c43b in std::allocator_traits<std::allocator<unsigned char>>::deallocate(std::allocator<unsigned char>&, unsigned char*, unsigned long) /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/alloc_traits.h:649:13
+ #6 0x55d1e4f1c43b in std::_Vector_base<unsigned char, std::allocator<unsigned char>>::_M_deallocate(unsigned char*, unsigned long) /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/stl_vector.h:396:4
+ #7 0x55d1e4f1c3ce in std::_Vector_base<unsigned char, std::allocator<unsigned char>>::~_Vector_base() /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/stl_vector.h:375:2
+ #8 0x55d1e4f18230 in std::vector<unsigned char, std::allocator<unsigned char>>::~vector() /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/stl_vector.h:805:7
+ #9 0x55d1e503d839 in std::vector<unsigned char, std::allocator<unsigned char>>::_M_move_assign(std::vector<unsigned char, std::allocator<unsigned char>>&&, std::integral_constant<bool, true>) /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/stl_vector.h:2266:7
+ #10 0x55d1e50280f4 in std::vector<unsigned char, std::allocator<unsigned char>>::operator=(std::vector<unsigned char, std::allocator<unsigned char>>&&) /usr/bin/../lib64/gcc/x86_64-suse-linux/15/../../../../include/c++/15/bits/stl_vector.h:838:2
+ #11 0x55d1e505984a in wabt::CodeMetadataExpr::CodeMetadataExpr(std::basic_string_view<char, std::char_traits<char>>, std::vector<unsigned char, std::allocator<unsigned char>>, wabt::Location const&) /home/wabt/include/wabt/ir.h:723:15
When using the assignment operator of the LHS, its implementation (in this case of std::vector) wrongfully assumes a valid object state of the LHS and, therefore, accesses uninitialized memory. That's why we're only allowed to call the assignment operator on objects in a proper state, which the LHS is not. Therefore, we use the placement new operator to fully overwrite the memory associated with the (non-existent) LHS object.
| std::vector<CallTarget> call_targets; | ||
| Hint() {} | ||
| ~Hint() {} | ||
| } hint; |
There was a problem hiding this comment.
Since we need to overwrite the constructor, we need to name the union, afaik. But please correct me if there's a way to define a destructor for an anonymous union in C++.
Title says it all, this PR adds support for compilation hints.