FFmpeg icon indicating copy to clipboard operation
FFmpeg copied to clipboard

Use -Werror and -Wpedantic in CI

Open frankplow opened this issue 1 year ago • 4 comments

This patchset does two things:

  • Adds the -Werror flag for only the libavcodec/vvc/*.o objects. This turns any compilation warnings for these files into CI failures.
  • Adds the -Wpedantic flag, which adds warnings whenever non-standard C features are used. This should help catch errors such as that fixed by https://github.com/FFmpeg/FFmpeg/commit/75e3b81f75b778286db7257a0ec4af74913ea67f

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

frankplow avatar Jun 27 '24 19:06 frankplow

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

frankplow avatar Jun 28 '24 09:06 frankplow

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

nuomi2021 avatar Jun 28 '24 13:06 nuomi2021

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

Unfortunately there does not appear to be any easy way to do this. There are no command line flags for GCC to selectively disable some ISO compliance checks, either it tests if the code is ISO C compliant or not. The only workaround would be to use pragmas to selectively disable -Wpedantic for only the problematic lines, but we'd have to patch this in and the diff would break with nearly any change to the code so I don't think this is workable. I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

frankplow avatar Jun 28 '24 16:06 frankplow

I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

If this costs too much time, it may not be worth it. Focus on fuzz may help us more.

But if you want, we can change https://github.com/ffvvc/FFmpeg/blob/e81b6d78fc2ddf8edd53a6a052713354ef8d27c2/libavcodec/vvc/vvc_intra_template.c#L241 to 1d array like pixel sel[VVC_MAX_SAMPLE_ARRAYS * MAX_PICK_POS * 2];

Thank you

nuomi2021 avatar Jun 30 '24 04:06 nuomi2021