[Bug default/19608] New: Need basic semantic checks for suppression files

[Bug default/19608] New: Need basic semantic checks for suppression files

[michi.henning at canonical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=19608

            Bug ID: 19608
           Summary: Need basic semantic checks for suppression files
           Product: libabigail
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: michi.henning at canonical dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

I erroneously concluded that the entire --suppression option was broken due to
the lack of feedback about things like parsing errors and the like in the
suppression file.

I'd strongly suggest to add basic sanity checks. For starters, I'd show any
errors from the ini parser. It would also be good to check for invalid group
and key names. Otherwise, one single typo is enough to potentially send me
scratching my head for hours.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/19608] Need basic semantic checks for suppression files

[michi.henning at canonical dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=19608

--- Comment #1 from Michi Henning <michi.henning at canonical dot com> ---
I just made this mistake:

[suppress_function]
    name_regexp = .*
    change_kind = added_function

Please, *please* add semantic checks. I spent five minutes trying to figure out
why this wasn't working.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/19608] Need basic semantic checks for suppression files

[dodji at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=19608

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/19608] Need basic semantic checks for suppression files

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=19608

Giuliano Procida <gprocida+abigail at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gprocida+abigail at google dot com

--- Comment #2 from Giuliano Procida <gprocida+abigail at google dot com> ---
I posted this proposal to the list in
https://sourceware.org/pipermail/libabigail/2020q2/002160.html.

I'm adding it here (with minor typos fixed) as it is directly relevant.

Note that some aspects of the proposal (such as error-checking regexes on
receipt and changing the way presence/absence information is held) will require
breaking changes to the API exposed in abg-suppression.h.

# Overview

This is a proposal to improve the parsing of suppression
specifications so that errors are detected and reported to users.

It is intended to resolve [this bug].

# Background

Suppression handling in libabigail can be broken down into:

* file format parsing
* suppression specification parsing
* suppression specification glue logic and internal suppression generation
* suppression evaluation (either at load time or diff time)

## INI Config

Suppression specifications are given in an ini config file format:
named sections containing key = value properties. While most
configuration values are plain strings, some richer value types (such
as tuples) are used in some cases. The ini config representation can
also be reserialised to text.

If an error occurs, the parser will typically return a null value
rather than something garbled. There are no error messages.

There is very little direct test coverage. However, there is indirect
testing via suppression specification and KMI whitelist parsing.

## Suppression Specifications

Parsing from ini config sections to suppression specifications is mostly
done by code in abg-suppression.cc. There is also some code, relating
to function_call_expr, that lives (but probably should not) in
abg-ini.{h,cc}.

There is a large amount of duplicated parsing code. Every duplicated
code path is an extra place for bugs to hide and that needs tests to
cover. Changes are costlier and riskier the more places that need to
be touched.

There is no parsing error checking. Most bad configuration is skipped
silently. In the case of regexes, they are compiled on first use and,
if they fail to compile, recompiled on every use but silently
ignored. Similarly, there are no checks on internally-generated
specifications.

The section parsers look for known properties in the input rather than
looking at the input and seeing if a given property is known or not.
Unrecognised (misspelled) properties are skipped silently.

Presence / absence information is handled in a various different ways
for different types (strings etc.) but for enumerated values (and
booleans), there are multiple different approaches, including bundling
enums with presence booleans.

There is a small amount of other logic mixed in with the parsing
logic, generally extra field validation or manipulation of fields.

## Suppression Evaluation

Mostly out of scope for this proposal. However, some of the parsing
issues translate over to the how suppressions are evaluated: code
duplication and inconsistent handling of similar things. There is
diverse interpretation of presence / absence information. In general,
it is *hard* to reason how a suppression specification with a few
related properties will behave in practice.

# Proposal

## High level goals

- error checking and reporting
- improved maintainability
- greater (proportional) test coverage
- bug fixes
- fewer surprising behaviours

## Plan for ini parser

(Plan A) Audit the ini parser, add diagnostic messages (to std::cerr)
and add more tests. The bundled code for parsing "function call
expressions" should be moved elsewhere. Fix the known string escaping
bug in the ini printer so that parse(print(x)) = x.

(Plan B) Consider whether a drop-in replacement using an existing
library would be worth it. The interface might be very different and
impose a significant integration cost. A new library may have its own
issues too.

## Internally-generated specifications

Add basic error checks (for regex compilation).

## Suppression specification parser

Every parsing function must return a parse success status and check
the status of parsing operations it itself performs.

All clear errors should generate a diagnostic message and result in a
parse error being returned. Useless or overridden configuration should
generate warning messages.

Aggressively refactor the code. Every piece of common parsing code
must be factored out into its own documented function which ideally
should be unit tested to the point that each success and failure case
is exercised.

Make presence / absence handling more uniform. For a given component
of a suppression specification it should be clear what the user intent
was after parsing is complete. In practice this would mean either
documenting that empty string values are equivalent to absent values
or changing the data representation from string to something with an
explicit absence value like unique_ptr<string> or optional<string>.

Separate miscellaneous logic from parsing logic and review it. There
is a limited amount of logic (relating to things like "drop"
properties) but it should be separate from the main parsing logic.
There will be opportunities to tighten or add further semantic checks
and changes once parsing is simplified.

Consider whether suppression specifications should be completely
devoid of logic and if so, investigate turning them into plain structs.

Switch to a scheme where the parser has a global view of known /
unknown property names so that errors caused by typos can be detected
and reported. Such a scheme must be data-driven (to avoid duplication
of code).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug default/19608] Need basic semantic checks for suppression files

[gprocida+abigail at google dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=19608

--- Comment #3 from Giuliano Procida <gprocida+abigail at google dot com> ---
Updating here in case this information is useful to someone.

I prepared a significant set of changes to add error handling and reporting to
the parser. Any such work has the potential to considerably bloat the code so
the changes aggressively refactored the parser to the point that, as far as
possible, there was no repetition in the error handling paths.

The upstream maintainer, however, doesn't favour this approach. Conversely, I
don't have the time or inclination to add error handling etc. in a way which
doesn't eliminate repetition. I am abandoning this line of development. Things
might change if there is strong demand for this feature.

The current version of my changes are on Github. I will continue to rebase them
onto master so long as there are no significant conflicts with other changes to
suppression parsing.

The changes up to

https://github.com/myxoid/libabigail/commits/suppression-parsing-error-handling

represent one approach to threading error status through all the parsing code.
If someone wants to add error handling in some other way, then these commits at
least show the extent of what needs to be done. Monadic error handling in
mainstream C++? Perhaps one day.

The changes up to

https://github.com/myxoid/libabigail/commits/suppression-parsing-errors

add error reporting (just logging to stderr).

The changes to

https://github.com/myxoid/libabigail/commits/suppression-parsing-warnings

add warnings for some cases where user-supplied information is useless or
ignored for one reason or another.

The changes up to

https://github.com/myxoid/libabigail/commits/suppression-parsing-speculative

contain some unfinished and/or speculative changes to the suppression parsing
code.

-- 
You are receiving this mail because:
You are on the CC list for the bug.