[Bug default/25998] New: Support incomplete enum types

[Bug default/25998] New: Support incomplete enum types

[gprocida+abigail at google dot com]

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

            Bug ID: 25998
           Summary: Support incomplete enum types
           Product: libabigail
           Version: unspecified
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P2
         Component: default
          Assignee: dodji at redhat dot com
          Reporter: gprocida+abigail at google dot com
                CC: libabigail at sourceware dot org
  Target Milestone: ---

Incomplete enum types are a GCC and Clang extension for C and are widely used
in the (AOSP at least) Linux kernel code.

C++ now supports "opaque" enums which are essentially forward-declared types
*of known size*.

If would good to have support in libabigail for the C case as this lack is
affecting AOSP kernel ABI monitoring.

I posted an RFC patch series demonstrating feasibility as:

https://sourceware.org/pipermail/libabigail/2020q2/001994.html

I followed up with a simplification of WIP types in the DWARF reader to reduce
some of the code duplication in the initial series:

https://sourceware.org/pipermail/libabigail/2020q2/002262.html

However, this only tackles a fraction of the duplication. There are two
possible approaches in general: templating over the relevant types or factoring
the common features into or otherwise using a type hierarchy.

libabigail already has a type hierarchy. There are at least two issues:

* the functionality for dealing with incomplete types exists at the
class_or_union level which is quite far down the hierarchy
* the common ancestors (plural, multiple inheritance) of class_or_union and
enum_decl_type are quite high up in hierarchy

Essentially, if we're going to try inheritance to avoid duplication, the
hierarchy will have to change.

Inheritance may not be a perfect fit, for example, there are separate
hierarchies of type types and diff types (and the diff types have to have
specialised methods like first_num) and things like
get_definition_of_declaration whose return value will need to be downcast if
it's made a base class method.

Templating comes with its own concerns. The main reason not to use it here is
that the combination of both inheritance and templating may impose a high
cognitive load on the poor developers.

Any detailed proposal should include full support for the newer C++ enum types
(scoped and opaque) and the should cover DWARF reader TODOs about supporting
incomplete union types.

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

[Bug default/25998] Support incomplete enum types

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-05-19
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from dodji at redhat dot com ---
Hello,

Thank you for filling this enhancement request and for working on its
underlying feature.

I agree with you that the incomplete enum support is needed and I am glad that
you started working on it.  I want to help get it in, so I am looking at it.

As for the way to go, I tend to not be a fan of early optimization.  So really,
at first, I wouldn't spend to much time (and I'd rather not spent too much
review time) on trying to remove duplicated code etc.

I'd rather want to have the feature in and make it work solidly first.  Of
course, if the design is elegant enough to integrate with the code base in a
manner that avoids duplication, then fine.  But I don't think we should go out
of our way to try and squeeze all the juice out of avoiding duplication.

Incidentally, I have posted a comment on the WIP types patches at
https://sourceware.org/pipermail/libabigail/2020q2/002262.html.

I'll be looking at the C++ enum type specification again, as I don't have the
details in mind at the moment.

Thanks again!

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

[Bug default/25998] Support incomplete enum types

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #2 from dodji at redhat dot com ---
I have amended the patches of your latest series at
https://sourceware.org/pipermail/libabigail/2020q2/002311.html.

You can see them stacked in my dodji/incomp-enums branch at
https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/dodji/incomp-enums.

The branch should be merge ready, but maybe you want to review/test it in your
environment first and depending on your feedback, we'll apply to master?

Thanks for working on this!

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

[Bug default/25998] Support incomplete enum types

[dodji at redhat dot com]

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

dodji at redhat dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |FIXED

--- Comment #3 from dodji at redhat dot com ---
This feature has been merged into the master branch of the git repository at
https://sourceware.org/git/?p=libabigail.git;a=commit;h=dbef379a9678c971a35ad477214464b2f7e08c7a.

Thank you for working on this!

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

[Bug default/25998] Support incomplete enum types

[gprocida+abigail at google dot com]

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

--- Comment #4 from Giuliano Procida <gprocida+abigail at google dot com> ---
I got some feedback from the partner who originally reported the issue.

"I saw that the [2 enum types] were being properly described in ABI report
now."

So this will fix their reported issue. We'll push a next libabigail release for
Android kernels in about a week.

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

[Bug default/25998] Support incomplete enum types

[dodji at redhat dot com]

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

--- Comment #5 from dodji at redhat dot com ---
"gprocida+abigail at google dot com"
<sourceware-bugzilla@sourceware.org> writes:

> https://sourceware.org/bugzilla/show_bug.cgi?id=25998
>
> --- Comment #4 from Giuliano Procida <gprocida+abigail at google dot com> ---
> I got some feedback from the partner who originally reported the issue.
>
> "I saw that the [2 enum types] were being properly described in ABI report
> now."

Houra! :-)

> So this will fix their reported issue.

Nice to hear.  Thanks for letting me know this.

> We'll push a next libabigail release for Android kernels in about a
> week.

OK.

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