[FFmpeg-devel] matroskadec: Add sizes to forward declarations

Submitted by Andreas Rheinhardt on July 17, 2019, 3:29 a.m.

Details

Message ID 20190717032940.48332-1-andreas.rheinhardt@gmail.com
State Accepted
Commit ab4795a085cd3deacb5e4bbaf527d66171361024
Headers show

Commit Message

Andreas Rheinhardt July 17, 2019, 3:29 a.m.
Unknown-length elements end when an element not allowed in them, but
allowed at a higher level is encountered. In order to check for this,
c1abd95a added a pointer to every syntax level's parent to each
EbmlSyntax. Given that the parent must of course also reference the
child in order to be able to enter said child level, one needs to use
forward declarations.
These forward declarations constitute tentative definitions and tentative
definitions with internal linkage (like our syntaxes) must not be an
incomplete type. Yet they were an incomplete type and while GCC and
Clang did not even warn about this (on default warning levels), it
broke compilation with MSVC. Therefore this commit adds the sizes.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
1. I am terribly sorry for this. I did test with both GCC and Clang, but
not with -pedantic, but only with default warning levels.
2. I don't have an MSVC setup here, so could please someone confirm that
this patch indeed fixes the compilation? All I know is that GCC's
-pedantic warnings are gone with this patch and that the requirement
that tentative definitions with internal linkage must not be of
incomplete type is fulfilled now.
 
 libavformat/matroskadec.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Paul B Mahol July 17, 2019, 3:24 p.m.
On 7/17/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Unknown-length elements end when an element not allowed in them, but
> allowed at a higher level is encountered. In order to check for this,
> c1abd95a added a pointer to every syntax level's parent to each
> EbmlSyntax. Given that the parent must of course also reference the
> child in order to be able to enter said child level, one needs to use
> forward declarations.
> These forward declarations constitute tentative definitions and tentative
> definitions with internal linkage (like our syntaxes) must not be an
> incomplete type. Yet they were an incomplete type and while GCC and
> Clang did not even warn about this (on default warning levels), it
> broke compilation with MSVC. Therefore this commit adds the sizes.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> 1. I am terribly sorry for this. I did test with both GCC and Clang, but
> not with -pedantic, but only with default warning levels.
> 2. I don't have an MSVC setup here, so could please someone confirm that
> this patch indeed fixes the compilation? All I know is that GCC's
> -pedantic warnings are gone with this patch and that the requirement
> that tentative definitions with internal linkage must not be of
> incomplete type is fulfilled now.
>
>  libavformat/matroskadec.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index b97189e674..fb0356e7b7 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -389,12 +389,16 @@ typedef struct MatroskaDemuxContext {
>
>  #define CHILD_OF(parent) { .def = { .n = parent } }
>
> -static const EbmlSyntax ebml_syntax[], matroska_segment[],
> matroska_track_video_color[], matroska_track_video[],
> -                        matroska_track[], matroska_track_encoding[],
> matroska_track_encodings[],
> -                        matroska_track_combine_planes[],
> matroska_track_operation[], matroska_tracks[],
> -                        matroska_attachments[], matroska_chapter_entry[],
> matroska_chapter[], matroska_chapters[],
> -                        matroska_index_entry[], matroska_index[],
> matroska_tag[], matroska_tags[], matroska_seekhead[],
> -                        matroska_blockadditions[], matroska_blockgroup[],
> matroska_cluster_parsing[];
> +// The following forward declarations need their size because
> +// a tentative definition with internal linkage must not be an
> +// incomplete type (6.7.2 in C90, 6.9.2 in C99).
> +// Removing the sizes breaks MSVC.
> +static const EbmlSyntax ebml_syntax[3], matroska_segment[9],
> matroska_track_video_color[15], matroska_track_video[19],
> +                        matroska_track[27], matroska_track_encoding[6],
> matroska_track_encodings[2],
> +                        matroska_track_combine_planes[2],
> matroska_track_operation[2], matroska_tracks[2],
> +                        matroska_attachments[2], matroska_chapter_entry[9],
> matroska_chapter[6], matroska_chapters[2],
> +                        matroska_index_entry[3], matroska_index[2],
> matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
> +                        matroska_blockadditions[2], matroska_blockgroup[8],
> matroska_cluster_parsing[8];
>
>  static const EbmlSyntax ebml_header[] = {
>      { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml, version),
>      { .u = EBML_VERSION } },
> --
> 2.21.0
>

Looks fine.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Hendrik Leppkes July 18, 2019, 7:26 a.m.
On Wed, Jul 17, 2019 at 5:35 AM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Unknown-length elements end when an element not allowed in them, but
> allowed at a higher level is encountered. In order to check for this,
> c1abd95a added a pointer to every syntax level's parent to each
> EbmlSyntax. Given that the parent must of course also reference the
> child in order to be able to enter said child level, one needs to use
> forward declarations.
> These forward declarations constitute tentative definitions and tentative
> definitions with internal linkage (like our syntaxes) must not be an
> incomplete type. Yet they were an incomplete type and while GCC and
> Clang did not even warn about this (on default warning levels), it
> broke compilation with MSVC. Therefore this commit adds the sizes.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> 1. I am terribly sorry for this. I did test with both GCC and Clang, but
> not with -pedantic, but only with default warning levels.
> 2. I don't have an MSVC setup here, so could please someone confirm that
> this patch indeed fixes the compilation? All I know is that GCC's
> -pedantic warnings are gone with this patch and that the requirement
> that tentative definitions with internal linkage must not be of
> incomplete type is fulfilled now.
>
>  libavformat/matroskadec.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index b97189e674..fb0356e7b7 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -389,12 +389,16 @@ typedef struct MatroskaDemuxContext {
>
>  #define CHILD_OF(parent) { .def = { .n = parent } }
>
> -static const EbmlSyntax ebml_syntax[], matroska_segment[], matroska_track_video_color[], matroska_track_video[],
> -                        matroska_track[], matroska_track_encoding[], matroska_track_encodings[],
> -                        matroska_track_combine_planes[], matroska_track_operation[], matroska_tracks[],
> -                        matroska_attachments[], matroska_chapter_entry[], matroska_chapter[], matroska_chapters[],
> -                        matroska_index_entry[], matroska_index[], matroska_tag[], matroska_tags[], matroska_seekhead[],
> -                        matroska_blockadditions[], matroska_blockgroup[], matroska_cluster_parsing[];
> +// The following forward declarations need their size because
> +// a tentative definition with internal linkage must not be an
> +// incomplete type (6.7.2 in C90, 6.9.2 in C99).
> +// Removing the sizes breaks MSVC.
> +static const EbmlSyntax ebml_syntax[3], matroska_segment[9], matroska_track_video_color[15], matroska_track_video[19],
> +                        matroska_track[27], matroska_track_encoding[6], matroska_track_encodings[2],
> +                        matroska_track_combine_planes[2], matroska_track_operation[2], matroska_tracks[2],
> +                        matroska_attachments[2], matroska_chapter_entry[9], matroska_chapter[6], matroska_chapters[2],
> +                        matroska_index_entry[3], matroska_index[2], matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
> +                        matroska_blockadditions[2], matroska_blockgroup[8], matroska_cluster_parsing[8];
>
>  static const EbmlSyntax ebml_header[] = {
>      { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml, version),         { .u = EBML_VERSION } },


Fixes compilaton on MSVC and FATE passes. Will apply soon if noone
beats me to it.

As an additional note, this section still causes a lot of warnings
because "const" is repeated. The typedef for EbmlSyntax is already
const, so this evaluates to "const const" and makes it throw warnings
about that. Maybe you can remove one of those in a follow up?

- Hendrik

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index b97189e674..fb0356e7b7 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -389,12 +389,16 @@  typedef struct MatroskaDemuxContext {
 
 #define CHILD_OF(parent) { .def = { .n = parent } }
 
-static const EbmlSyntax ebml_syntax[], matroska_segment[], matroska_track_video_color[], matroska_track_video[],
-                        matroska_track[], matroska_track_encoding[], matroska_track_encodings[],
-                        matroska_track_combine_planes[], matroska_track_operation[], matroska_tracks[],
-                        matroska_attachments[], matroska_chapter_entry[], matroska_chapter[], matroska_chapters[],
-                        matroska_index_entry[], matroska_index[], matroska_tag[], matroska_tags[], matroska_seekhead[],
-                        matroska_blockadditions[], matroska_blockgroup[], matroska_cluster_parsing[];
+// The following forward declarations need their size because
+// a tentative definition with internal linkage must not be an
+// incomplete type (6.7.2 in C90, 6.9.2 in C99).
+// Removing the sizes breaks MSVC.
+static const EbmlSyntax ebml_syntax[3], matroska_segment[9], matroska_track_video_color[15], matroska_track_video[19],
+                        matroska_track[27], matroska_track_encoding[6], matroska_track_encodings[2],
+                        matroska_track_combine_planes[2], matroska_track_operation[2], matroska_tracks[2],
+                        matroska_attachments[2], matroska_chapter_entry[9], matroska_chapter[6], matroska_chapters[2],
+                        matroska_index_entry[3], matroska_index[2], matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
+                        matroska_blockadditions[2], matroska_blockgroup[8], matroska_cluster_parsing[8];
 
 static const EbmlSyntax ebml_header[] = {
     { EBML_ID_EBMLREADVERSION,    EBML_UINT, 0, offsetof(Ebml, version),         { .u = EBML_VERSION } },