<div dir="ltr"><div class="gmail_default" style="font-family:monospace;font-size:small;color:#000000">Sounds good to me too.   +kcc@ to comment on the reuse/refactoring of  sanitizer_common library for common use.</div><div class="gmail_default" style="font-family:monospace;font-size:small;color:#000000"><br></div><div class="gmail_default" style="font-family:monospace;font-size:small;color:#000000">David</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 15, 2021 at 10:14 AM Vedant Kumar <<a href="mailto:vsk@apple.com">vsk@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><blockquote type="cite">On Jul 14, 2021, at 5:50 PM, Petr Hosek <<a href="mailto:phosek@google.com" target="_blank">phosek@google.com</a>> wrote:<br></blockquote><div><blockquote type="cite"><br><div><div dir="ltr"><div>That renaming makes sense given that sanitizer_common is slowly becoming a common runtime library. At the risk of more bikeshedding, we could also consider naming it just 'common'. We might also consider breaking up sanitizer_common into multiple libraries like 'common', 'libcdep', 'symbolizer', 'sancov', matching the existing object libraries (see <a href="https://github.com/llvm/llvm-project/blob/0da172b/compiler-rt/lib/sanitizer_common/CMakeLists.txt" target="_blank">https://github.com/llvm/llvm-project/blob/0da172b/compiler-rt/lib/sanitizer_common/CMakeLists.txt</a>).</div><div><br></div><div>I want to avoid introducing external dependencies (either direct or transitive via sanitizer_common), in fact my goal is the opposite, I want to eliminate the existing dependency on libc. This is important for us since we use the profile runtime in our kernel and our libc. The use of C++ as an implementation language shouldn't be a problem, we have plenty of experience using C++ in bare metal contexts.</div></div></div></blockquote><div><br></div><div>Sounds good, I think we're on the same page.</div><br><blockquote type="cite"><div><div dir="ltr"><div>From the user perspective this change should be invisible and the libclang_rt.profile ABI should ideally remain the same. We can introduce tooling to enforce that by comparing libclang_rt.profile ABI against a list that's checked into the repository, which is an approach that other runtimes use as well.</div></div></div></blockquote><div><br></div><div>This would be great to have. At the moment, we only have profile runtime tests that certain symbols *aren't* used/exported (instrprof-{darwin-exports,without-libc}.c).</div><div><br></div><div>thanks,</div><div>vedant</div><br><blockquote type="cite"><div><div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 14, 2021 at 5:19 PM Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-family:monospace;font-size:small">I actually meant 'runtime_common'.</div><div style="font-family:monospace;font-size:small"><br></div><div style="font-family:monospace;font-size:small">David</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 14, 2021 at 5:07 PM Vedant Kumar <<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><blockquote type="cite">On Jul 14, 2021, at 4:56 PM, Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:<br></blockquote><div><blockquote type="cite"><br><div><div dir="ltr"><div style="font-family:monospace;font-size:small">Can we introduce C wrappers to sanitizer_common and use them in profile runtime? Of course 'sanitizer_common' needs to become 'profile_common'.</div></div></div></blockquote><div><br></div><div>I like the sound of 'runtime_common' a bit better (sorry for the bikeshed :).</div><div><br></div><div>vedant</div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div style="font-family:monospace;font-size:small"><br></div><div style="font-family:monospace;font-size:small">David</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 14, 2021 at 4:14 PM Petr Hosek <<a href="mailto:phosek@google.com" target="_blank">phosek@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">What are your thoughts on using C++ and sanitizer_common in profile runtime?<div><br></div><div>First, to clarify, I'm not suggesting using the standard C++ library. What I'm suggesting is just using the C++ language akin to sanitizer runtimes or memprof.</div><div><br></div><div>profile runtime is the last major runtime in compiler-rt that's implemented in C and switching to C++ could make the compiler-rt codebase more uniform. Beyond that, I think this could be beneficial for several reasons:</div><div><br></div><div>* _Reducing the duplication between profile runtime and sanitizer_common._ Both of these implement various polyfills for different platforms (for example the mmap like interface) and it would be great if we could avoid duplicating the effort and share a common implementation.</div><div><br></div><div>* _Avoiding potential cyclic dependencies in profile runtime._ The implementation currently uses various libc functions, but if libc is profile instrumented there is a risk of potentially entering an infinite recursion. This could be avoided by using internal implementations of these libc functions which sanitizer_common does, but if we wanted to take the same approach for profile runtime, we would need to re-implement these functions again further increasing duplication.</div><div><br></div><div>* _Simplifying the profile runtime implementation._ Some parts of the runtime already use object-oriented style interfaces, that is structs with function pointers, replacing these with classes would make the code cleaner. Furthermore, we could use classes to abstract away some of the platform implementation which currently relies on ifdefs. We could also take the advantage of RAII for resource management.</div><div><br></div><div>If we decided to go ahead with this change, I'd suggest an incremental transition rather than doing a major rewrite. We would start by ensuring that all source files compile as C++. Then we would introduce the sanitizer_common dependency and see which functions could be replaced by sanitizer_common counterparts. Finally, we could refactor the implementation and take advantage of C++ constructs where appropriate.</div></div>
</blockquote></div>
</div></blockquote></div><br></div></blockquote></div>
</blockquote></div></div>
</div></blockquote></div><br></div></blockquote></div>