Add Clang Thread Safety Annotations

[jiridanek]

Follows up on

• Add Clang Thread Safety Annotations (locks-only) #1036

It can supplement

• Runtime Debug tests to validate proper thread execution #1015

Proposal, aiming to partially provide the features of apache/qpid-dispatch#805, at compile time.

Uses https://clang.llvm.org/docs/ThreadSafetyAnalysis.html (with all the limitations that stem from it, namely, works only in clang or IDE based on clangd, and the GUARDED_BY annotation cannot be used in C structs, llvm/llvm-project#20777)

This is really a draft, but I'd nevertheless appreciate somebody looking at it and giving a preliminary review

Calling a _LH function without holding the lock

Unlocking something that is not locked

Returning without unlocking

TODO

Similarly, if there are particular routines that should only be called by the GUI thread, then the analysis will warn if other threads call those routines.

This could be applied to annotate _CT functions. Docs does not say exactly how to do this. I have a guess...

[jiridanek]

This one is licensed under Apache, https://github.com/google/google-input-tools/blob/master/client/base/thread_annotations.h

[kgiusti]

I'm in favor of the idea, but would like others to weigh in since it will require all contributors to be aware of these constructs while working on lock-related code.

I'm going to request reviews from some of the proton devs as well since we've involved them in the past regarding how best to interact with the proactor threading model.

[jiridanek]

Issues I discovered so far

suppressing the warning for a single line

Here is one way. The main problem is that GCC will not accept #pragma clang annotations (although that warning can be suppressed, this may be actually the cleanest way to deal with this). The clang is happy to accept #pragma GCC, so that's what can be used. The suppression for the thread safety warning is clang specific, so there has to be a conditional; thanks to c11, it is possible to put a pragma into a macro

#ifdef __clang__
#define TA_SUPPRESS _Pragma("clang diagnostic ignored \"-Wthread-safety-analysis\"")
#else
#define TA_SUPPRESS
#endif

usage is

    // initialize the Q2 flag. Note that we do not need to hold the content
    // lock when we make this call since no other threads are able to access
    // the new message until this function returns.
#pragma GCC diagnostic push
    TA_SUPPRESS;
    if (_Q2_holdoff_should_block_LH(content))
        content->q2_input_holdoff = true;
#pragma GCC diagnostic pop

(Suppressing for entire function is easy, just annotate with TA_NO_THREAD_SAFETY_ANALYSIS.)

annotating a function which does not have its lock in the scope is impossible

In the following, there are two functions which must be protected by LOCK(content->lock);, but they don't take content as an argument, so there is no way to annotate them in the function declaration.

• line 2752: find_last_buffer_LH(&stream_data->section, &msg->body_cursor, &msg->body_buffer);
• line 2754: trim_stream_data_headers_LH(stream_data, !is_footer);

In case of trim_stream_data_headers_LH It is possible to get at the lock through stream_data, but that is not good enough for -Wthread-safety-precise (see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#warning-flags)

skupper-router/src/message.c

Lines 2724 to 2758 in cfb31ce

	LOCK(content->lock);
	section_status = message_section_check_LH(content,
	&msg->body_buffer, &msg->body_cursor,
	BODY_DATA_SHORT, 3, TAGS_BINARY,
	&location,
	true, // allow duplicates
	false); // do not inc buffer fanout
	if (section_status == QD_SECTION_NO_MATCH) {
	is_footer = true;
	section_status = message_section_check_LH(content,
	&msg->body_buffer, &msg->body_cursor,
	FOOTER_SHORT, 3, TAGS_MAP,
	&location, true, false);
	}
	switch (section_status) {
	case QD_SECTION_INVALID:
	case QD_SECTION_NO_MATCH:
	result = QD_MESSAGE_STREAM_DATA_INVALID;
	break;
	case QD_SECTION_MATCH:
	stream_data = new_qd_message_stream_data_t();
	ZERO(stream_data);
	stream_data->owning_message = msg;
	stream_data->section = location;
	stream_data->first_buffer = start_buffer;
	find_last_buffer_LH(&stream_data->section, &msg->body_cursor, &msg->body_buffer);
	stream_data->last_buffer = msg->body_buffer;
	trim_stream_data_headers_LH(stream_data, !is_footer);
	DEQ_INSERT_TAIL(msg->stream_data_list, stream_data);
	*out_stream_data = stream_data;
	result = is_footer ? QD_MESSAGE_STREAM_DATA_FOOTER_OK : QD_MESSAGE_STREAM_DATA_BODY_OK;
	break;

limitations

there are limitations described here and at the clang doc site; one example is that calls through function pointers are not checked

alternatives

• dynamic checking, some custom macro, but then the annotation would have to be in function body, not before the {, so one annotation cannot serve both for clang and this.
• coverity, see the section "C/C++ Concurrency Primitives" in manual at https://url.corp.redhat.com/cov-customizing; so far I haven't found a way there how to assert lock must be held ;(

[jiridanek]

Found the coverity usage, it is in that page, I knew I saw it there before, I just did not realize the second time

#ifdef __COVERITY__
    extern void __coverity_recursive_lock_acquire__(void*);
    extern void __coverity_recursive_lock_release__(void*);
    extern void __coverity_assert_locked__(void*);
#endif

// ...

sal_Bool SAL_CALL osl_releaseMutex(oslMutex pMutex)
{
#ifdef __COVERITY__
    __coverity_assert_locked__(pMutex);
#endif

Example from https://dev-builds.libreoffice.org/cppcheck_reports/master/3633.html

[jiridanek]

Calling _CT function without being on the core_thread

[codecov]

Codecov Report

Merging #212 (a57a466) into main (2469371) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #212   +/-   ##
=====================================
  Coverage   77.7%   77.7%           
=====================================
  Files        247     247           
  Lines      64011   64010    -1     
  Branches    5896    5896           
=====================================
+ Hits       49751   49771   +20     
+ Misses     11565   11551   -14     
+ Partials    2695    2688    -7     

Flag	Coverage Δ
pysystemtests	87.7% <ø> (-0.1%)	⬇️
pyunittests	54.6% <ø> (ø)
systemtests	71.3% <100.0%> (+<0.1%)	⬆️
unittests	21.9% <65.2%> (-0.1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
unittests	26.0% <65.2%> (-0.1%)	⬇️
systemtests	78.4% <100.0%> (+<0.1%)	⬆️