abi-compliance-checker
Attached is a html report that was generated by a tool called abi-compliance-checker/abi-dumper [1][2] (by using "abi-compliance-checker -l libTest ... ") . I deliberately introduced 2 ABI compatibility issues affecting postgres, just to see what the tool had to say about it. The first ABI issue I mocked up involved a breaking change to the signature of a function with external linkage. Sure enough, this issue (in CheckForSerializableConflictIn(), as it happens) appears in the report as a medium severity item. The second ABI issue I mocked up involved "filling-in" a hole in a struct (a struct that appears in a header that could be included by an extension) with a new field. In other words, the "put new field in existing alignment padding" trick. This kind of difference is generally believed to be non-breaking, and so is acceptable in point releases. But the issue still appears as a low severity item in the report. The report points out (quite reasonably) that my newly added field won't be initialized by old code. In most cases this will be fine, of course. It's just not something that should be taken for granted. Overall, I like the report format -- especially its severity scale. So it seems like abi-compliance-checker has the potential to become a standard release management tool for Postgres point releases. I can imagine a community resource along the lines of https://coverage.postgresql.org; an automatically generated archive of theoretical/actual x86_64 ABI breaks in each point release. I'd appreciate having greater visibility into these issues. [1] https://github.com/lvc/abi-dumper [2] https://manpages.debian.org/unstable/abi-dumper/abi-dumper.1.en.html -- Peter Geoghegan Title: libTest: X to Y compatibility report API compatibility report between postgres (X) and postgres (Y) objects on x86_64 BinaryCompatibility SourceCompatibility Test Info Module NamelibTest Version #1X Version #2Y Archx86_64 GCC Version12.2.0 SubjectBinary Compatibility Test Results Total Header Files606 Total Source Files808 Total Objects1 Total Symbols / Types10461 / 3416 Compatibility 100% Problem Summary SeverityCountAdded Symbols-0 Removed SymbolsHigh0 Problems withData TypesHigh0 Medium0 Low1 Problems withSymbolsHigh0 Medium1 Low0 Problems withConstantsLow0 Problems with Symbols, Medium Severity 1 predicate.h, postgres [+] CheckForSerializableConflictIn ( Relation relation, ItemPointer tid, BlockNumber blkno ) 1 ⇣ CheckForSerializableConflictIn ( Relation relation, ItemPointer tid, BlockNumber blkno, int foo ) Change Effect 1 Parameter foo of type int has been added to the calling stack. This parameter will not be initialized by old clients. to the top Problems with Data Types, Low Severity 1 genam.h [+] struct IndexVacuumInfo 1 Change Effect 1 Field fill_hole has been added to this type. This field will not be initialized by old clients.NOTE: this field should be accessed only from the new library functions, otherwise it may result in crash or incorrect behavior of applications. [+] affected symbols: 876 (8.4%) _bt_allequalimage ( Relation rel, _Bool debugmessage ) Field 'rel.rd_indam.ambulkdelete.p0' in 1st parameter 'rel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_allocbuf ( Relation rel, Relation heapRel ) Field 'heapRel.rd_indam.ambulkdelete.p0' in 2nd parameter 'heapRel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_binsrch_insert ( Relation rel, BTInsertState insertstate ) Field 'rel.rd_indam.ambulkdelete.p0' in 1st parameter 'rel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_bottomupdel_pass ( Relation rel, Buffer buf, Relation heapRel, Size newitemsz ) Field 'heapRel.rd_indam.ambulkdelete.p0' in 3rd parameter 'heapRel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_check_natts ( Relation rel, _Bool heapkeyspace, Page page, OffsetNumber offnum ) Field 'rel.rd_indam.ambulkdelete.p0' in 1st parameter 'rel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_check_third_page ( Relation rel, Relation heap, _Bool needheaptidspace, Page page, IndexTuple newtup ) Field 'heap.rd_indam.ambulkdelete.p0' in 2nd parameter 'heap' (pointer) has base type 'struct IndexVacuumInfo'. _bt_checkpage ( Relation rel, Buffer buf ) Field 'rel.rd_indam.ambulkdelete.p0' in 1st parameter 'rel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_compare ( Relation rel, BTScanInsert key, Page page, OffsetNumber offnum ) Field 'rel.rd_indam.ambulkdelete.p0' in 1st parameter 'rel' (pointer) has base type 'struct IndexVacuumInfo'. _bt_conditionallockbuf ( Relation rel, Buffer buf ) Field 'rel.rd_indam.ambulkde
Re: abi-compliance-checker
On 2023-Nov-01, Peter Eisentraut wrote: > v3-0001-abidw-option.patch > > This adds the abidw meson option, which produces the xml files with the ABI > description. With that, you can then implement a variety of workflows, such > as the abidiff proposed in the later patches, or something rigged up via CI, > or you can just build various versions locally and compare them. With this > patch, you get the files to compare built automatically and don't have to > remember to cover all the libraries or which options to use. > > I think this patch is mostly pretty straightforward and agreeable, subject > to technical review in detail. I like this idea and I think we should integrate it with the objective of it becoming the workhorse of ABI-stability testing. However, I do not think that the subsequent patches should be part of the tree at all; certainly not the produced .xml files in your 0004, as that would be far too unstable and would cause a lot of pointless churn. > TODO: documentation Yes, please. > TODO: Do we want a configure/make variant of this? Not needed IMO. The way I see this working, is that we set up a buildfarm animal [per architecture] that runs the new rules produced by the abidw option and stores the result locally, so that for stable branches it can turn red when an ABI-breaking change with the previous minor release of the same branch is introduced. There's no point on it ever turning red in the master branch, since we're obviously not concerned with ABI changes there. (Perhaps we do need 0003 as an easy mechanism to run the comparison, but I'm not sure to what extent that would be actually helpful.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: abi-compliance-checker
On Tue, Feb 27, 2024 at 8:25 AM Alvaro Herrera wrote: > The way I see this working, is that we set up a buildfarm animal [per > architecture] that runs the new rules produced by the abidw option and > stores the result locally, so that for stable branches it can turn red > when an ABI-breaking change with the previous minor release of the same > branch is introduced. There's no point on it ever turning red in the > master branch, since we're obviously not concerned with ABI changes there. ABI stability doesn't seem like something that you can alert on. There are quite a few individual cases where the ABI was technically broken, in a way that these tools will complain about. And yet it was generally understood that these changes did not really break ABI stability, for various high-context reasons that no tool can possibly be expected to understand. This will at least be true under our existing practices, or anything like them. For example, if you look at the "Problems with Symbols, High Severity" from the report I posted comparing REL_11_0 to REL_11_20, you'll see that I removed _bt_pagedel() when backpatching a fix. That was justified by the fact that any extension that was calling that function was already hopelessly broken (I pointed this out at the time). Having some tooling in this area would be extremely useful. The absolute number of false positives seems quite manageable, but the fact is that most individual complaints that the tooling makes are false positives. At least in some deeper sense. -- Peter Geoghegan
Re: abi-compliance-checker
On 2024-Feb-27, Peter Geoghegan wrote: > On Tue, Feb 27, 2024 at 8:25 AM Alvaro Herrera > wrote: > > The way I see this working, is that we set up a buildfarm animal [per > > architecture] that runs the new rules produced by the abidw option and > > stores the result locally, so that for stable branches it can turn red > > when an ABI-breaking change with the previous minor release of the same > > branch is introduced. There's no point on it ever turning red in the > > master branch, since we're obviously not concerned with ABI changes there. > > ABI stability doesn't seem like something that you can alert on. Eh, I disagree. Since you can add suppression rules to the tree, I'd say it definitely is. If you commit something and it breaks ABI, we want to know as soon as possible -- for example suppose the ABI break occurs during a security patch at release time; if we get an alert about it immediately, we still have time to fix it before the mess is released. Now, if you have an intentional ABI break, then you can let the testing system know about it so that it won't complain. We could for example have src/abi/suppressions/REL_11_8.ini and src/abi/suppressions/REL_12_3.ini files (in the respective branches) with the _bt_pagedel() change. You can add this file together with the commit that introduces the change, if you know about it ahead of time, or as a separate commit after the buildfarm animal turns red. Or you can fix your ABI break, if -- as is most likely -- it was unintentional. Again -- this only matters for stable branches. We don't need to do anything for the master branch, as it would be far too noisy if we did that. Now, maybe a buildfarm animal is not the right tool, and instead we need a separate system that tests for it and emails pg-hackers when it breaks or whatever. That's fine with me, but it seems a pretty minor implementation detail. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: abi-compliance-checker
On Tue, Feb 27, 2024 at 9:03 AM Alvaro Herrera wrote: > Now, maybe a buildfarm animal is not the right tool, and instead we need > a separate system that tests for it and emails pg-hackers when it breaks > or whatever. That's fine with me, but it seems a pretty minor > implementation detail. Anything that alerts on breakage is pretty much equivalent to having a buildfarm animal. I have a feeling that there are going to be real problems with alerting, at least if it's introduced right away. I'd feel much better about it if there was an existing body of suppressions, that more or less worked as a reference of agreed upon best practices. Can we do that part first, rather than starting out with a blanket assumption that everything that happened before now must have been perfect? -- Peter Geoghegan
Re: abi-compliance-checker
On 2024-Feb-27, Peter Geoghegan wrote: > I have a feeling that there are going to be real problems with > alerting, at least if it's introduced right away. I'd feel much better > about it if there was an existing body of suppressions, that more or > less worked as a reference of agreed upon best practices. Can we do > that part first, rather than starting out with a blanket assumption > that everything that happened before now must have been perfect? Well, I was describing a possible plan, not saying that we have to assume we've been perfect all along. I think the first step should be to add the tooling now (Meson rules as in Peter E's 0001 patch upthread, or something equivalent), then figure out what suppressions we need in the supported back branches. This would let us build the corpus of best practices you want, I think. Once we have clean runs with those, we can add BF animals or whatever. The alerts don't have to be the first step. In fact, we can wait even longer for the alerts. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: abi-compliance-checker
On 27.02.24 14:25, Alvaro Herrera wrote: I like this idea and I think we should integrate it with the objective of it becoming the workhorse of ABI-stability testing. However, I do not think that the subsequent patches should be part of the tree at all; certainly not the produced .xml files in your 0004, as that would be far too unstable and would cause a lot of pointless churn. Looking at this again, if we don't want the .xml files in the tree, then we don't really need this patch series. Most of the delicate work in the 0001 patch was to find the right abidw options combinations to reduce the variability in the .xml output files (--no-show-locs is an obvious example). If we don't want to record the .xml files in the tree, then we don't need all these complications. For example, if we want to check the postgres backend ABI across minor versions, we could just compile it multiple times and compare the binaries directly: git checkout REL_16_0 meson setup build-0 meson compile -C build-0 git checkout REL_16_STABLE meson setup build-1 meson compile -C build-1 abidiff --no-added-syms build-0/src/backend/postgres build-1/src/backend/postgres The way I see this working, is that we set up a buildfarm animal [per architecture] that runs the new rules produced by the abidw option and stores the result locally, so that for stable branches it can turn red when an ABI-breaking change with the previous minor release of the same branch is introduced. There's no point on it ever turning red in the master branch, since we're obviously not concerned with ABI changes there. Maybe the way forward here is to write a buildfarm module for this, and then see from there what further needs there are.
Re: abi-compliance-checker
On Mon, Mar 4, 2024 at 7:50 AM Peter Eisentraut wrote: > Looking at this again, if we don't want the .xml files in the tree, then > we don't really need this patch series. Based on this, I've updated the status of this patch in the CommitFest application to Withdrawn. If that's not correct, please feel free to adjust. -- Robert Haas EDB: http://www.enterprisedb.com
Re: abi-compliance-checker
On Wed, 1 Nov 2023 at 16:43, Peter Eisentraut wrote: > > Here is an updated version of this patch. It doesn't have any new > functionality, just a rebase and some minor adjustments. > > I have split up the one patch into several ones, which could be > considered incrementally, namely: > > v3-0001-abidw-option.patch > > This adds the abidw meson option, which produces the xml files with the > ABI description. With that, you can then implement a variety of > workflows, such as the abidiff proposed in the later patches, or > something rigged up via CI, or you can just build various versions > locally and compare them. With this patch, you get the files to compare > built automatically and don't have to remember to cover all the > libraries or which options to use. > > I think this patch is mostly pretty straightforward and agreeable, > subject to technical review in detail. > > TODO: documentation > TODO: Do we want a configure/make variant of this? > > v3-0002-Enable-abidw-option-on-Cirrus-CI.patch > > This adds the abidw option to some CI tasks. This was mostly used by me > during development to get feedback from other machines and to produce > base files for the subsequent abidiff patch. I'm not sure whether we > need it in isolation (other than for general testing that the option > works at all). > > v3-0003-abidiff-option.patch > > This adds the abidiff test suite that compares base files previously > produced with the abidw option to the currently built libraries. There > is clearly some uncertainty here whether the produced files are stable > enough, whether we want this particular workflow, what additional > burdens this would create, etc., so I'm not hung up on this right now, > it's mostly a demonstration. > > v3-0004-abidiff-support-files.patch > > This contains the support files for patch 0003, just split out because > they are bulky and boring. One of the test has failed in cfbot at [1] with: abi-compliance-checker [12:04:10.537] The output from the failed tests: [12:04:10.537] [12:04:10.537] 129/282 postgresql:abidiff / plpgsql.abidiff FAIL 1.25s (exit status 4) [12:04:10.537] [12:04:10.537] --- command --- [12:04:10.537] 12:03:00 /usr/bin/abidiff /tmp/cirrus-ci-build/build/../src/pl/plpgsql/src/plpgsql.x86_64-linux.abi.xml src/pl/plpgsql/src/plpgsql.so [12:04:10.537] --- Listing only the last 100 lines from a long log. --- [12:04:10.537] 'NodeTag::T_RoleSpec' from value '66' to '67' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_FuncCall' from value '67' to '68' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Star' from value '68' to '69' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Indices' from value '69' to '70' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Indirection' from value '70' to '71' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_ArrayExpr' from value '71' to '72' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_ResTarget' from value '72' to '73' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_MultiAssignRef' from value '73' to '74' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_SortBy' from value '74' to '75' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_WindowDef' from value '75' to '76' at nodes.h:26:1 [12:04:10.592] --- [12:04:10.592] [12:04:10.592] [12:04:10.592] Summary of Failures: [12:04:10.592] [12:04:10.592] 129/282 postgresql:abidiff / plpgsql.abidiff FAIL 1.25s (exit status 4) [1] - https://cirrus-ci.com/task/5961614579466240 Regards, Vignesh
Re: abi-compliance-checker
On 06.01.24 18:25, vignesh C wrote: One of the test has failed in cfbot at [1] with: abi-compliance-checker [12:04:10.537] The output from the failed tests: [12:04:10.537] [12:04:10.537] 129/282 postgresql:abidiff / plpgsql.abidiff FAIL 1.25s (exit status 4) [12:04:10.537] [12:04:10.537] --- command --- [12:04:10.537] 12:03:00 /usr/bin/abidiff /tmp/cirrus-ci-build/build/../src/pl/plpgsql/src/plpgsql.x86_64-linux.abi.xml src/pl/plpgsql/src/plpgsql.so [12:04:10.537] --- Listing only the last 100 lines from a long log. --- [12:04:10.537] 'NodeTag::T_RoleSpec' from value '66' to '67' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_FuncCall' from value '67' to '68' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Star' from value '68' to '69' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Indices' from value '69' to '70' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_Indirection' from value '70' to '71' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_A_ArrayExpr' from value '71' to '72' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_ResTarget' from value '72' to '73' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_MultiAssignRef' from value '73' to '74' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_SortBy' from value '74' to '75' at nodes.h:26:1 [12:04:10.537] 'NodeTag::T_WindowDef' from value '75' to '76' at nodes.h:26:1 [12:04:10.592] --- [12:04:10.592] [12:04:10.592] [12:04:10.592] Summary of Failures: [12:04:10.592] [12:04:10.592] 129/282 postgresql:abidiff / plpgsql.abidiff FAIL 1.25s (exit status 4) [1] - https://cirrus-ci.com/task/5961614579466240 This is kind of intentional, as it shows the the test catches ABI changes. If the patches were to be committed, then the base ABI file would be updated.
Re: abi-compliance-checker
On 10.06.23 22:24, Andres Freund wrote: Looks like we ought to add --exported-interfaces-only? Btw., this option requires libabigail 2.1, which isn't available everywhere yet. For example, Debian oldstable (used on Cirrus) doesn't have it yet. So I'll leave this patch set as is for now. If it turns out that this is the right option and we want to proceed with this patch set, we might need to think about a version check or something.
Re: abi-compliance-checker
Peter Geoghegan writes: > Attached is a html report that was generated by a tool called > abi-compliance-checker/abi-dumper [1][2] (by using > "abi-compliance-checker -l libTest ... ") . I deliberately introduced > 2 ABI compatibility issues affecting postgres, just to see what the > tool had to say about it. This seems pretty cool. I agree that we're in dire need of some tool of this sort for checking back-branch patches. I wonder though if it'll have false-positive problems. Have you tried it on live rather than mocked-up cases, for instance 13.0 vs 13.11? regards, tom lane
Re: abi-compliance-checker
Peter Geoghegan writes: > I tried comparing REL_11_0 to REL_11_20. Attached is the report for that. Nice! > I don't have time to study this in detail today, but the report seems > to have a plausible variety of issues. I noticed that it warns about > the breaking signature change to _bt_pagedel(). This is the > theoretical ABI break that I mentioned in the commit message of commit > b0229f26. This is arguably a false positive, since the tool doesn't > understand my reasoning about why it's okay in this particular > instance (namely "any extension that called that function was already > severely broken"). Obviously the tool couldn't possibly be expected to > know better in these kinds of situations, though, so whether or not it > counts as a false positive is just semantics. Agreed. The point of such a tool is to make sure that we notice any ABI breaks; it can't be expected to make engineering judgments about whether the alternatives are worse. For instance, I see that it noticed commit 1f28ec6be (Rename rbtree.c functions to use "rbt" prefix not "rb" prefix), which is not something we would have done of our own choosing, but on balance it seemed the best solution. I gather it'd catch things like NodeTag enum assignments changing, which is something we really need to have a check for. (Which reminds me that I forgot to turn on the ad-hoc check for that in gen_node_support.pl. I'll go do that, but it'd be better to have a more general-purpose solution.) regards, tom lane
Re: abi-compliance-checker
I wrote: > (Which reminds me that I forgot to turn on the ad-hoc check for > that in gen_node_support.pl. I'll go do that, but it'd be better > to have a more general-purpose solution.) Oh, scratch that, it's not supposed to happen until we make the v16 branch. It'd still be better to not need it. regards, tom lane
Re: abi-compliance-checker
On Sun, May 28, 2023 at 8:37 AM Tom Lane wrote: > I gather it'd catch things like NodeTag enum assignments changing, > which is something we really need to have a check for. Right. Any ABI break that involves machine-generated translation units seems particularly prone to being overlooked. Just eyeballing code (and perhaps double-checking struct layout using pahole) seems inadequate. I'll try to come up with a standard abi-compliance-checker Postgres workflow once I'm back from pgCon. It already looks like abi-compliance-checker is capable of taking most of the guesswork out of ABI compatibility in stable releases, without any real downside, which is encouraging. I have spent very little time on this, so it's quite possible that some detail or other was overlooked. -- Peter Geoghegan
Re: abi-compliance-checker
On Sun, May 28, 2023 at 9:34 AM Peter Geoghegan wrote: > I'll try to come up with a standard abi-compliance-checker Postgres > workflow once I'm back from pgCon. Ideally, we'd be able to produce reports that cover an entire stable release branch at once, including details about how things changed over time. It turns out that there is a tool from the same family of tools as abi-compliance-checker, that can do just that: https://github.com/lvc/abi-tracker There is an abi-tracker example report, here: https://abi-laboratory.pro/?view=timeline&l=glib It's exactly the same presentation as the report I posted recently, once you drill down. That seems ideal. -- Peter Geoghegan
Re: abi-compliance-checker
On 27.05.23 02:52, Peter Geoghegan wrote: Attached is a html report that was generated by a tool called abi-compliance-checker/abi-dumper [1][2] (by using "abi-compliance-checker -l libTest ... ") . I have been using the libabigail library/set of tools (abidiff, abidw) for this. I was not familiar with the tool you used. The nice thing about abidiff is that it gives you text output and a meaningful exit status, so you can make it part of the build or test process. You can also write suppression files to silence specific warnings. I think the way to use this would be to compute the ABI for the .0 release (or rc1 or something like that) and commit it into the tree. And then compute the current ABI and compare that against the recorded base ABI. Here is the workflow: # build REL_11_0 abidw src/backend/postgres > src/tools/postgres-abi-REL_11_0.xml # build REL_11_20 abidw src/backend/postgres > src/tools/postgres-abi.xml abidiff --no-added-syms src/tools/postgres-abi-REL_11_0.xml src/tools/postgres-abi.xml This prints Functions changes summary: 0 Removed, 0 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Function symbols changes summary: 14 Removed, 0 Added (85 filtered out) function symbols not referenced by debug info Variable symbols changes summary: 1 Removed, 0 Added (3 filtered out) variable symbols not referenced by debug info 14 Removed function symbols not referenced by debug info: [D] RelationHasUnloggedIndex [D] assign_nestloop_param_placeholdervar [D] assign_nestloop_param_var [D] logicalrep_typmap_gettypname [D] logicalrep_typmap_update [D] pqsignal_no_restart [D] rb_begin_iterate [D] rb_create [D] rb_delete [D] rb_find [D] rb_insert [D] rb_iterate [D] rb_leftmost [D] transformCreateSchemaStmt 1 Removed variable symbol not referenced by debug info: [D] wrconn This appears to be similar to what your tool produced, but I haven't checked it in detail.
Re: abi-compliance-checker
+abidiff = find_program('abidiff', native: false, required: false)
+abidw = find_program('abidw', native: false, required: false)
+
+abidw_flags = [
+ '--drop-undefined-syms',
+ '--no-architecture',
+ '--no-comp-dir-path',
+ '--no-elf-needed',
+ '--no-show-locs',
+ '--type-id-style', 'hash',
+]
+abidw_cmd = [abidw, abidw_flags, '--out-file', '@OUTPUT@', '@INPUT@']
It would make sense to me to mark abidiff and abidw as disabler: true.
+if abidw.found()
+ libpq_abi = custom_target('libpq.abi.xml',
+input: libpq_so,
+output: 'libpq.abi.xml',
+command: abidw_cmd,
+build_by_default: true)
+endif
+
+if abidiff.found()
+ test('libpq.abidiff',
+ abidiff,
+ args: [files('libpq.base.abi.xml'), libpq_abi],
+ suite: 'abidiff',
+ verbose: true)
+endif
With disabler: true, you can drop the conditionals. Disablers tell Meson
to disable parts of the build[0].
I also don't think it makes sense to mark the custom_targets as
build_by_default: true, unless you see value in that. I would just have
them built when the test is ran.
However, it might make sense to create an alias_target of all the ABI
XML files for people that want to interact with the files outside of the
tests for whatever reason.
[0]: https://mesonbuild.com/Reference-manual_returned_disabler.html
--
Tristan Partin
Neon (https://neon.tech)
Re: abi-compliance-checker
Hi, On 2023-06-06 18:30:38 +0200, Peter Eisentraut wrote: > On 30.05.23 06:32, Peter Eisentraut wrote: > > I think the way to use this would be to compute the ABI for the .0 > > release (or rc1 or something like that) and commit it into the tree. And > > then compute the current ABI and compare that against the recorded base > > ABI. > > > > Here is the workflow: > > > > # build REL_11_0 > > abidw src/backend/postgres > src/tools/postgres-abi-REL_11_0.xml > > # build REL_11_20 > > abidw src/backend/postgres > src/tools/postgres-abi.xml > > abidiff --no-added-syms src/tools/postgres-abi-REL_11_0.xml > > src/tools/postgres-abi.xml > > Here is a demo patch that implements this. > > Right now, I have only added support for libpq and postgres. For > completeness, the ecpg libraries should be covered as well. I think plpgsql would also be good to include, due to things like plpgsql debuggers. > * Different Linux distributions produce slightly different ABI reports. In > some cases, symbols like 'optarg@GLIBC_2.17' leak out. Hm, that's somewhat annoying. > * PostgreSQL compilation options affect the exposed ABI. This is perhaps > expected to some degree, but there are some curious details. > > * For example, --enable-cassert exposes additional symbols, and it's maybe > not impossible for those to leak into an extension. They *definitely* leak into extensions. A single Assert() in an extension or use of an inline function or macro with an Assertion suffices to end up with a reference to ExceptionalCondition. > diff --git a/src/interfaces/libpq/libpq.base.abi.xml > b/src/interfaces/libpq/libpq.base.abi.xml > new file mode 100644 > index 00..691bf192af > --- /dev/null > +++ b/src/interfaces/libpq/libpq.base.abi.xml > @@ -0,0 +1,2634 @@ > + > + > + binding='global-binding' visibility='default-visibility' is-defined='yes'/> > + binding='global-binding' visibility='default-visibility' is-defined='yes'/> > + visibility='default-visibility' is-defined='yes'/> > [...] > + binding='global-binding' visibility='default-visibility' is-defined='yes'/> > + This seems somewhat painful in its verbosity. We also effectively already have it in the tree, in src/interfaces/libpq/exports.txt. But I guess that's somewhat inevitable :/ It sounds we are planning to mostly rely on CI for this, perhaps we should rely on an artifact from a prior build for a major version + specific task, instead of committing this to source? That'd automatically take care of differences in ABI across different platforms etc. If we want to commit something to the tree, I think we need a fairly complicated "fingerprint" to avoid false positives. OS, OS version, configure options, compiler, compiler version at least? > + language='LANG_C99'> > + id='752c85d9'> > + > + > + size-in-bits='infinite' id='ac835593'> > + > + > + id='728d2ee1'> > + > + > + size-in-bits='infinite' id='a01b33bb'> > + > + > + > + visibility='default' id='79f06fd8'> > + > + > + > + > + > + > + > + > + > + Hm - why is all of this stuff even ending up in the external ABI? It should all be internal, unless I am missing something? I might be looking the wrong way, but to me it sure looks like none of that ends up being externally visible? Greetings, Andres Freund
Re: abi-compliance-checker
Hi, On 2023-06-10 12:48:46 -0700, Andres Freund wrote: > > + > > + > > + > > Hm - why is all of this stuff even ending up in the external ABI? It should > all be internal, unless I am missing something? > > I might be looking the wrong way, but to me it sure looks like none of that > ends up being externally visible? Looks like we ought to add --exported-interfaces-only? That still seems to include things that shouldn't be there, but much less. E.g.: and things outside of our control: I guess the latter would have to be suppressed via suppression file. But I don't understand why things like AddrInfo ends up being included... I tried using --header-file with --drop-private-types. But that ends up dropping all enum definitions for some reason. Independently, I'm a bit confused as to why we export pgresStatus in exports.txt - I don't see any reason for that. Looks like it might be leftover from before fa0f24165c0? We're also a bit schizophrenic about where we install pqexpbuffer.h - includedir_internal. But at the same time we export all the symbols? Greetings, Andres Freund
Re: abi-compliance-checker
Andres Freund writes: > Independently, I'm a bit confused as to why we export pgresStatus in > exports.txt - I don't see any reason for that. Looks like it might be leftover > from before fa0f24165c0? It looks like before fa0f24165, the *only* way to convert ExecStatusType to text was to access that array directly. That commit invented the wrapper function PQresStatus(), but at that point our docs were so poor that there wasn't any good way to mark use of the array as deprecated. A bit later, 9ceb5d8a7 moved the array declaration to libpq-int.h (without any discussion in the commit message, but maybe there was some on-list). Maybe there's still application code out there using it, I dunno. What I do know is that removing the exports.txt entry will provoke squawks from distros' ABI checkers. regards, tom lane
Re: abi-compliance-checker
On Sat Jun 10, 2023 at 9:17 AM CDT, Peter Eisentraut wrote: > I have rearranged this a bit. There are now two build options, called > abidw and abidiff. The abidw option produces the xml file, that you > would then at appropriate times commit into the tree as the base. The > abidiff option enables the abidiff tests. This doesn't actually require > abidw, since abidiff can compare the binary directly against the > recorded XML file. So these options are distinct and nonoverlapping. > > Note that in this setup, you still need a conditional around the abidiff > test() call, because otherwise meson setup will fail if the base file > doesn't exist (yet), so it would be impossible to bootstrap this system. Could you speak more to the workflow you see with managing the checked in diff files? At my previous job, I had tried to do something similar with regard to making sure we didn't break ABI[0], but I took a different approach where instead of hooking into the Meson test infrastructure, I used a CI job where I checked out the previous major version of the code and the current version of the code, built both, and checked the built binaries. The benefit of this workflow is that you don't check anything into the source repo. I think the same approach might be better here, but instead of writing it all into the CI file like I did, use a perl script. Then once you have the perl script, it could be possible to then hook into the Meson test infrastructure. > There is something weird going on where the cirrus linux/meson job > doesn't upload the produced abidw artifacts, even though they are > apparently built, and the equivalent works for the freebsd job. Maybe > someone can see something that I'm not seeing there. Nothing obvious is wrong to me. Was the failure maybe just a fluke? [0]: https://github.com/hse-project/hse/blob/master/.github/workflows/abicheck.yaml -- Tristan Partin Neon (https://neon.tech)
Re: abi-compliance-checker
On Mon Jun 12, 2023 at 10:10 AM CDT, Tristan Partin wrote: > On Sat Jun 10, 2023 at 9:17 AM CDT, Peter Eisentraut wrote: > > I have rearranged this a bit. There are now two build options, called > > abidw and abidiff. The abidw option produces the xml file, that you > > would then at appropriate times commit into the tree as the base. The > > abidiff option enables the abidiff tests. This doesn't actually require > > abidw, since abidiff can compare the binary directly against the > > recorded XML file. So these options are distinct and nonoverlapping. > > > > Note that in this setup, you still need a conditional around the abidiff > > test() call, because otherwise meson setup will fail if the base file > > doesn't exist (yet), so it would be impossible to bootstrap this system. > > Could you speak more to the workflow you see with managing the checked > in diff files? Just saw your other email which talks about the workflow. -- Tristan Partin Neon (https://neon.tech)
