llvm dev - Jul 2021 - Using C++ and sanitizer_common in profile runtime

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
https://github.com/llvm/llvm-project/blob/0da172b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
).

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.
>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.

On Wed, Jul 14, 2021 at 5:19 PM Xinliang David Li <davidxl at google.com>
wrote:
> I actually meant 'runtime_common'.
>
> David
>
> On Wed, Jul 14, 2021 at 5:07 PM Vedant Kumar <vsk at apple.com>
wrote:
>
>> On Jul 14, 2021, at 4:56 PM, Xinliang David Li <davidxl at
google.com>
>> wrote:
>>
>>
>> Can we introduce C wrappers to sanitizer_common and use them in profile
>> runtime? Of course 'sanitizer_common' needs to become
'profile_common'.
>>
>>
>> I like the sound of 'runtime_common' a bit better (sorry for
the bikeshed
>> :).
>>
>> vedant
>>
>>
>>
>> David
>>
>> On Wed, Jul 14, 2021 at 4:14 PM Petr Hosek <phosek at google.com>
wrote:
>>
>>> What are your thoughts on using C++ and sanitizer_common in profile
>>> runtime?
>>>
>>> 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.
>>>
>>> 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:
>>>
>>> * _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.
>>>
>>> * _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.
>>>
>>> * _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.
>>>
>>> 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.
>>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210714/06712292/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3996 bytes
Desc: S/MIME Cryptographic Signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210714/06712292/attachment.bin>

> On Jul 14, 2021, at 5:50 PM, Petr Hosek <phosek at google.com> wrote:
> 
> 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
https://github.com/llvm/llvm-project/blob/0da172b/compiler-rt/lib/sanitizer_common/CMakeLists.txt
<https://github.com/llvm/llvm-project/blob/0da172b/compiler-rt/lib/sanitizer_common/CMakeLists.txt>).
> 
> 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.
Sounds good, I think we're on the same page.
> 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.
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).

thanks,
vedant
> 
> On Wed, Jul 14, 2021 at 5:19 PM Xinliang David Li <davidxl at google.com
<mailto:davidxl at google.com>> wrote:
> I actually meant 'runtime_common'.
> 
> David
> 
> On Wed, Jul 14, 2021 at 5:07 PM Vedant Kumar <vsk at apple.com
<mailto:vsk at apple.com>> wrote:
>> On Jul 14, 2021, at 4:56 PM, Xinliang David Li <davidxl at
google.com <mailto:davidxl at google.com>> wrote:
>> 
>> Can we introduce C wrappers to sanitizer_common and use them in profile
runtime? Of course 'sanitizer_common' needs to become
'profile_common'.
> 
> I like the sound of 'runtime_common' a bit better (sorry for the
bikeshed :).
> 
> vedant
> 
> 
>> 
>> David
>> 
>> On Wed, Jul 14, 2021 at 4:14 PM Petr Hosek <phosek at google.com
<mailto:phosek at google.com>> wrote:
>> What are your thoughts on using C++ and sanitizer_common in profile
runtime?
>> 
>> 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.
>> 
>> 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:
>> 
>> * _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.
>> 
>> * _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.
>> 
>> * _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.
>> 
>> 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.
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210715/59f18d2f/attachment.html>