Skip to content

Conversation

@eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Sep 10, 2025

Prompted by #1447:

There is a lot of noise in the v_noabi API compatibility report due to v1 API being included in the unstable ABI report despite the list of headers to analyze being limited to v_noabi headers. The XML descriptors may need to be updated to better filter results to v_noabi API only.

Updates the abi-compliance-check script so that v1 API is not reported as part of the unstable ABI (v_noabi) API compatibility report. Although only /v_noabi/ headers are being included for analysis, because they transitively include /v1/ headers, v1 namespace entities are being included in the unstable ABI's API compatibility report. Furthermore, it unfortunately seems the namespaces must be listed relative to the root namespace:

if($NameSpace)
{ # user defined namespaces to ignore
    if($In::Desc{$LVer}{"SkipNameSpaces"}{$NameSpace}) {
        return 0;
    }
    foreach my $NS (keys(%{$In::Desc{$LVer}{"SkipNameSpaces"}}))
    { # nested namespaces
        if($NameSpace=~/\A\Q$NS\E(\:\:|\Z)/) { 
            return 0;
        }
    }
}

According to Perl docs, \A matches the "beginning of string" and \Z matches the "end of string". Therefore, a <skip_namespace> entry like bsoncxx::v1 will match (all relative to the root namespace) both bsoncxx::v1 and bsoncxx::v1::foo, but it will not match foo::bsoncxx::v1. It also seems the XML descriptors do not support reopening XML tags (i.e. <skip_namespaces>); therefore, the XML file generation is updated accordingly to support both stable vs. unstable and old vs. new properly.


There remains a confusing set of supposed potential API breaking changes to constructors in the API compatibility report for #1447, e.g. concerning v_noabi::types::bson_value::value:

"Removed Symbols":

image

"Problems with Symbols, Medium Severity":

image

I do not understand what is causing these "Removed" and "Changed" entries to be reported. As far as I can tell, these are related to the inlining of previously-exported constructors, which however should not be considered an API breaking change (though it is of course an ABI breaking change).

- BSONCXX_ABI_EXPORT_CDECL() value(std::chrono::milliseconds v);
+ /* explicit(false) */ value(std::chrono::milliseconds v) : _value{v} {}

The "removed" symbols are being reported as "parameter type changed" entries whose resulting mangled name is completely identical to the removed entry (there is no change in parameter types):

_ZN7bsoncxx7v_noabi5types10bson_value5valueC2ENSt6chrono8durationIlNSt5ratioILl1ELl1000EEEEE (removed)
_ZN7bsoncxx7v_noabi5types10bson_value5valueC2ENSt6chrono8durationIlNSt5ratioILl1ELl1000EEEEE (changed)

It is possible these are compiler internal representation details being misinterpreted as significant API breaking changes (failure to recognized the old vs. new symbols are the same)...? In other words, I do not believe these to be significant.

Note

The "[CN]" and "[DN]" alongside ctor/dtor symbols in the report where N is an integer are explained by the Itanium ABI specification concerning ctor/dtor mangling:

Constructors and destructors are simply special cases of <unqualified-name>, where the final <unqualified-name> of a nested name is replaced by one of the following:

<ctor-dtor-name> ::= C1                        # complete object constructor
                 ::= C2                        # base object constructor
                 ::= C3                        # complete object allocating constructor
                 ::= CI1 <base class type>     # complete object inheriting constructor
                 ::= CI2 <base class type>     # base object inheriting constructor
                 ::= D0                        # deleting destructor
                 ::= D1                        # complete object destructor
                 ::= D2                        # base object destructor

The <base class type> in an inheriting constructor mangling identifies the base class in which the inherited constructor was originally declared.

Some of the symbols for constructor and destructor variants are optional.

and concerning "optional":

This ABI does not require the generation or use of allocating constructors or inheriting constructors, and does not require the generation or use of deleting destructors for classes without a virtual destructor. However, if an implementation emits such functions, it must use the external names specified in this ABI. If such a function has external linkage, it must be emitted wherever referenced, in a COMDAT group whose name is the external name of the function.


Unrelated to changes in #1447, there is also this entry in the API compatibility report:

image

I do not understand what is causing this entry to be emitted either. I am unable to filter out this entry by adding std to <skip_namespaces>, and I'm not sure it's supposed be reported in the first place due to being an entity provided by a system header, thus it is not included in this PR. Given this is concerning the instantiation of a template whose definition should be visible (+ not owned by us anyways), I do not believe this is a significant issue either.

@eramongodb eramongodb self-assigned this Sep 10, 2025
@eramongodb eramongodb requested a review from a team as a code owner September 10, 2025 15:11
<skip_namespaces>
bsoncxx::detail
mongocxx::detail
Copy link
Collaborator

@kevinAlbs kevinAlbs Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm: is skipping bsoncxx::v_noabi unnecessary? I expect "yes" since v1 headers will not include v_noabi headers and /v_noabi/ is already included in skip_headers.

Copy link
Contributor Author

@eramongodb eramongodb Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct: unlike v_noabi headers which include v1 headers, v1 headers do not include v_noabi headers; the bsoncxx::v_noabi namespace is completely absent when only v1 headers are included.

@eramongodb eramongodb merged commit 031b33b into mongodb:master Sep 10, 2025
22 checks passed
@eramongodb eramongodb deleted the cxx-abi-compliance-check branch September 10, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants