Re: [PATCH v4 10/15] Compile suppression regexes earlier.

[Giuliano Procida <gprocida@google.com>]

Hi there.

On Wed, 13 May 2020 at 09:07, Dodji Seketeli <dodji@seketeli.org> wrote:

> Giuliano Procida <gprocida@google.com> a écrit:
>
> > Regexes are used in suppression specifications for various things.
> > They are currently compiled on first use, rather than on construction
> > of the specifications.
> >
> > This patches changes the suppression specification types to hold
> > compiled regexes directly which is a significant simplification and is
> > a prerequisite for early error reporting.
>
> [...]
>
> > diff --git a/include/abg-suppression.h b/include/abg-suppression.h
> > index 6383b932..8256f9b8 100644
> > --- a/include/abg-suppression.h
> > +++ b/include/abg-suppression.h
> > @@ -26,6 +26,7 @@
> >  #include "abg-cxx-compat.h"
> >  #include "abg-ini.h"
> >  #include "abg-comparison.h"
> > +#include "abg-regex.h"
> >
> >  namespace abigail
> >  {
> > @@ -64,8 +65,8 @@ public:
> >    suppression_base(const string& label);
> >
> >    suppression_base(const string& label,
> > -                const string& file_name_regex_str,
> > -                const string& file_name_not_regex_str);
> > +                const regex::regex_t_sptr& file_name_regex,
> > +                const regex::regex_t_sptr& file_name_not_regex);
>
> I am not against compiling the regular expressions early.
>
> However, I don't like leaking the regex_t type through the suppression
> API.  I'd really prefer the compiled regular expression type to stay
> internal to the suppression API.
>

Mostly as an aside, I considered whether it would be worth wrapping up
regex_t in a type that was more than just a shared_ptr with a specialised
destructor. While it would be nice to have a copyable value type, this is a
poor fit for regex_t which isn't copyable. The existing regex_t_sptr type
is OK but lacks a way to create new memset-to-0 regex_t objects and there
is no const-protecting version. We should really be looking ahead to when
we can switch to std::basic_regex or similar, wrapped in std::optional
when... optional. std::basic_regex throws an exception on construction if
the regex cannot be compiled. I think it would be reasonable to have this
standard library type as part of the suppression API going forward.

If the suppression API has to remain stable between major releases of
libabigail, then we can never remove things like:

// does not check validity of regexp
void set_file_name_not_regex_str(const string& regexp);
// allows nominally constant insertion ranges to be mutated
const insertion_ranges& get_data_member_insertion_ranges() const;

I do understand we can add always to the interface, but we may end up with
more code than we'd like to maintain.

In other words, existing users of the library who created suppressions
> just using regular expressions strings (as opposed to compiled regular
> expressions) to interact with the API should be able to continue to do
> so.  This breaks the API compatibility un-necessarily, IMHO.  I'd like
> to avoid that.  And I don't think it's a design flaw to interact with
> the API just through strings, as opposed to using it through regex_t,
> quite the opposite.  It was a design decision that I tried to stick to
> throughout the library.  The reader interface does't leak the underlying
> XML library types.  The dwarf reader doesn't leak the types of the
> underlying facilities we use, etc.  So I'd like this to stay consistent
> accross the library.
>

A large surface has been exposed in abg-suppression.h, including plenty of
things that could be considered implementation details. I might have
envisaged exposing a single function or method, something like:

  bool read_suppressions(std::istream& is, ...);

However, what is completely unclear to (Matthias and) me is: what users of
libabigail-as-a-library there are and what they might like to see in the
way of interface change or stability. Can you shed any light on this?

I am sorry to tell you this this late.  I could have saved you a lot of
> work; I didn't realize in our discussions that this was what you wanted
> to do.
>

We did discuss changing the internal representation of regexes (dropping
the string representation) and this implies dropping all the

  const string& get_foo_regex_str() const;

methods from the interface.

I also mentioned perhaps keeping the string representation around for error
reporting. But with early compilation of regexes, that buys very little.
There is one possible use-case: semantic checks on regexes and other
suppression properties.

I've already contributed abg-regex.h to the public interface.

But, never mind. I can look at reworking my patch series to preserve the
existing exposed API in abg-suppression.h. I predict considerable extra
complexity in two areas (regexes and insertion ranges) and some minor extra
complications in others (enums used for change kinds etc.). However, this
will not happen any time soon as I need to focus on bugs and features that
are more relevant to kernel ABI work.

It makes sense to consider the rest of the patches in the series posted to
the list as abandoned, though you might possibly want to adapt
"abg-tools-utils.cc: Assert generated regexes OK."


> Cheers,
>
> --
>                 Dodji
>

Regards,
Giuliano.


> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscribe@android.com.
>
>