Logs from building VDR 2.1.3 with Clang 3.4.1
Commit Message
On 08.02.2014 09:59, Paul Menzel wrote:
> Dear VDR folks,
>
>
> building VDR 2.1.3 with Clang 3.4.1 the warnings below are shown.
>
> Most warnings look like they can be ignored. Maybe you can spot
> something, which should be fixed.
>
> $ clang --version
> Debian clang version 3.4-1 (tags/RELEASE_34/final) (based on LLVM 3.4)
> Target: i386-pc-linux-gnu
> Thread model: posix
> $ CC=clang CXX=clang++ make
> […]
> clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated
You may want to find an option that suppresses this warning, because the file names of VDR (*.c)
are not going to change.
> ...
> channels.c:45:119: warning: data argument not used by format string [-Wformat-extra-args]
> snprintf(buffer, sizeof(buffer), rid ? "%s-%d-%d-%d-%d" : "%s-%d-%d-%d", *cSource::ToString(source), nid, tid, sid, rid);
> ~~~~~~~~~~~~~ ^
This is explicitly checked with 'rid ? ...', so the warning is unjustified (although the compiler
probably has a hard time figuring that out ;-).
> ...
> ci.c:867:18: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(...
> ^~~~~
> .mjd =
> ci.c:867:36: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(...
> ^~~
> .h =
> ci.c:867:65: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(...
> ^~~
> .m =
> ci.c:867:93: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(...
> ^~~
> .s =
> ci.c:867:121: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(tm_lo...
> ^~~~~~~~
> .offset =
> ci.c:1007:47: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tDisplayReply dr = { id : DRI_MMI_MODE_ACK, mode : MM_HIGH_LEVEL };
> ^~~~
> .id =
> ci.c:1007:70: warning: use of GNU old-style field designator extension [-Wgnu-designator]
> tDisplayReply dr = { id : DRI_MMI_MODE_ACK, mode : MM_HIGH_LEVEL };
> ^~~~~~
> .mode =
Fixed in the attached patch.
> ...
> In file included from dvbspu.c:14:
> ./dvbspu.h:104:10: warning: private field 'ready' is not used [-Wunused-private-field]
> bool ready;
> ^
Fixed in the attached patch.
> ...
> dvbsubtitle.c:37:13: warning: unused variable 'DebugBitmaps' [-Wunused-variable]
> static bool DebugBitmaps = DebugVerbose || DebugNormal;
> ^
Fixed in the attached patch.
> ...
> eit.c:223:43: warning: comparison of constant 176 with expression of type 'SI::LinkageType' is always false
> [-Wtautological-constant-out-of-range-compare]
> if (ld->getLinkageType() == 0xB0) { // Premiere World
> ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
I assume this is because the enum LinkageType doesn't contain 0xB0. However, the actual value that
comes from the SI data may well be 0xB0, so I'm now typecasting uint(ld->getLinkageType()).
> ...
> menu.c:982:38: warning: first operand of this 'memcmp' call is a pointer to dynamic class 'cTimer'; vtable pointer will be compared
> [-Wdynamic-class-memaccess]
> if (memcmp(timer, &data, sizeof(data)) != 0)
> ~~~~~~ ^
> menu.c:982:38: note: explicitly cast the pointer to silence this warning
> if (memcmp(timer, &data, sizeof(data)) != 0)
> ^
> (void*)
Fixed in the attached patch.
> menu.c:4582:107: warning: data argument not used by format string [-Wformat-extra-args]
> instantId = cString::sprintf(cDevice::NumDevices() > 1 ? "%s - %d" : "%s", timer->Channel()->Name(), device->CardIndex() + 1);
> ~~~~ ^
Same as above (channels.c:45:119).
> ...
> menuitems.c:249:86: warning: data argument not used by format string [-Wformat-extra-args]
> SetValue(cString::sprintf(s ? "%.*f %s" : "%.*f", factor / 10, double(v) / factor, s));
> ~~~~~~ ^
Same as above.
> ...
> receiver.c:28:6: warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
> *(char *)0 = 0; // cause a segfault
> ^~~~~~~~~~
> receiver.c:28:6: note: consider using __builtin_trap() or qualifying pointer with 'volatile'
Can you suggest a different way of causing a segfault at this point?
> ...
> recording.c:2923:96: warning: data argument not used by format string [-Wformat-extra-args]
> return cString::sprintf(WithFrame ? "%s%d:%02d:%02d.%02d" : "%s%d:%02d:%02d", Sign, h, m, s, f);
> ~~~~~~~~~~~~~~~~ ^
Same as above.
> ...
> In file included from remux.c:10:
> ./remux.h:479:7: warning: private field 'numFrames' is not used [-Wunused-private-field]
> int numFrames;
> ^
Fixed in the attached patch.
> ...
> In file included from descriptor.c:14:
> ./descriptor.h:493:34: warning: private field 's' is not used [-Wunused-private-field]
> const descr_iso_639_language *s;
> ^
Fixed in the attached patch.
> ...
> hdffosd.c:45:14: warning: private field 'mBitmapNumColors' is not used [-Wunused-private-field]
> uint32_t mBitmapNumColors;
> ^
> hdffosd.c:558:14: warning: private field 'mBitmapNumColors' is not used [-Wunused-private-field]
> uint32_t mBitmapNumColors;
> ^
Fixed in the attached patch.
> ...
> *** Plugin pictures:
> ...
> player.c:222:15: warning: case value not in enumerated type 'eKeys' [-Wswitch]
> case kLeft|k_Repeat:
> ^
> player.c:226:16: warning: case value not in enumerated type 'eKeys' [-Wswitch]
> case kRight|k_Repeat:
Fixed in the attached patch.
Klaus
Comments
On Sat, 08 Feb 2014 13:51:10 +0100
Klaus Schmidinger <Klaus.Schmidinger@tvdr.de> wrote:
> > channels.c:45:119: warning: data argument not used by format string [-Wformat-extra-args]
> > snprintf(buffer, sizeof(buffer), rid ? "%s-%d-%d-%d-%d" : "%s-%d-%d-%d", *cSource::ToString(source), nid, tid, sid, rid);
> > ~~~~~~~~~~~~~ ^
>
> This is explicitly checked with 'rid ? ...', so the warning is
> unjustified (although the compiler probably has a hard time figuring
> that out ;-).
The warning is justified, because if rid is 0 it's still there as an
argument, but just happens to have a value of 0. I think you can make
snprintf "consume" it without printing anything by adding %.d to the
second format string.
> > eit.c:223:43: warning: comparison of constant 176 with expression of type 'SI::LinkageType' is always false
> > [-Wtautological-constant-out-of-range-compare]
> > if (ld->getLinkageType() == 0xB0) { // Premiere World
> > ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
>
> I assume this is because the enum LinkageType doesn't contain 0xB0.
> However, the actual value that comes from the SI data may well be
> 0xB0, so I'm now typecasting uint(ld->getLinkageType()).
If 0xB0 has a significant meaning wouldn't it be a good idea to add it
to the enum?
> > receiver.c:28:6: warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
> > *(char *)0 = 0; // cause a segfault
> > ^~~~~~~~~~
> > receiver.c:28:6: note: consider using __builtin_trap() or qualifying pointer with 'volatile'
>
> Can you suggest a different way of causing a segfault at this point?
You could add volatile as the warning suggests. Is there a good reason
not to use abort() instead?
On 08.02.2014 14:34, Tony Houghton wrote:
> On Sat, 08 Feb 2014 13:51:10 +0100
> Klaus Schmidinger <Klaus.Schmidinger@tvdr.de> wrote:
>
>>> channels.c:45:119: warning: data argument not used by format string [-Wformat-extra-args]
>>> snprintf(buffer, sizeof(buffer), rid ? "%s-%d-%d-%d-%d" : "%s-%d-%d-%d", *cSource::ToString(source), nid, tid, sid, rid);
>>> ~~~~~~~~~~~~~ ^
>>
>> This is explicitly checked with 'rid ? ...', so the warning is
>> unjustified (although the compiler probably has a hard time figuring
>> that out ;-).
>
> The warning is justified, because if rid is 0 it's still there as an
> argument, but just happens to have a value of 0. I think you can make
> snprintf "consume" it without printing anything by adding %.d to the
> second format string.
I'm afraid not.
If I run
#include <stdio.h>
int main(void)
{
for (int n = 0; n < 2; n++)
printf(n ? "'%d-%d'\n" : "'%d%.d'\n", 1, 2);
return 0;
}
I get
'12'
'1-2'
But maybe there *is* such a format character, it just isn't "%.d".
>>> eit.c:223:43: warning: comparison of constant 176 with expression of type 'SI::LinkageType' is always false
>>> [-Wtautological-constant-out-of-range-compare]
>>> if (ld->getLinkageType() == 0xB0) { // Premiere World
>>> ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
>>
>> I assume this is because the enum LinkageType doesn't contain 0xB0.
>> However, the actual value that comes from the SI data may well be
>> 0xB0, so I'm now typecasting uint(ld->getLinkageType()).
>
> If 0xB0 has a significant meaning wouldn't it be a good idea to add it
> to the enum?
You're probably right. Since there are already several "Premiere" related definitions in
libsi/si.h I guess it won't hurt to add yet another one. <rant>The DVB standard should never have
allowed all this "private" stuff...</rant>
>>> receiver.c:28:6: warning: indirection of non-volatile null pointer will be deleted, not trap [-Wnull-dereference]
>>> *(char *)0 = 0; // cause a segfault
>>> ^~~~~~~~~~
>>> receiver.c:28:6: note: consider using __builtin_trap() or qualifying pointer with 'volatile'
>>
>> Can you suggest a different way of causing a segfault at this point?
>
> You could add volatile as the warning suggests. Is there a good reason
> not to use abort() instead?
The idea behind this was to allow for easy backtracking in such a case.
I believe abort() wouldn't allow this, would it?
Klaus
On Sat, 08 Feb 2014 15:17:09 +0100
Klaus Schmidinger <Klaus.Schmidinger@tvdr.de> wrote:
> On 08.02.2014 14:34, Tony Houghton wrote:
> >
> > The warning is justified, because if rid is 0 it's still there as an
> > argument, but just happens to have a value of 0. I think you can make
> > snprintf "consume" it without printing anything by adding %.d to the
> > second format string.
>
> I'm afraid not.
> If I run
>
> #include <stdio.h>
> int main(void)
> {
> for (int n = 0; n < 2; n++)
> printf(n ? "'%d-%d'\n" : "'%d%.d'\n", 1, 2);
> return 0;
> }
>
> I get
>
> '12'
> '1-2'
>
> But maybe there *is* such a format character, it just isn't "%.d".
You're right, it looks like it has to be %.s, it doesn't work with %.d.
If you used %.s you'd probably just get a type mismatch warning instead
:-(. There's nothing wrong with the way you wrote it, but I like to
enable all possible warnings and eliminate them. In this case I think
I'd just print the rid even if it's 0.
> >> Can you suggest a different way of causing a segfault at this point?
> >
> > You could add volatile as the warning suggests. Is there a good reason
> > not to use abort() instead?
>
> The idea behind this was to allow for easy backtracking in such a case.
> I believe abort() wouldn't allow this, would it?
abort() does usually generate a useful core dump/stack backtrace IME.
On 08.02.2014 16:10, Tony Houghton wrote:
> On Sat, 08 Feb 2014 15:17:09 +0100
> Klaus Schmidinger <Klaus.Schmidinger@tvdr.de> wrote:
>
>> On 08.02.2014 14:34, Tony Houghton wrote:
>>>
>>> The warning is justified, because if rid is 0 it's still there as an
>>> argument, but just happens to have a value of 0. I think you can make
>>> snprintf "consume" it without printing anything by adding %.d to the
>>> second format string.
>>
>> I'm afraid not.
>> If I run
>>
>> #include <stdio.h>
>> int main(void)
>> {
>> for (int n = 0; n < 2; n++)
>> printf(n ? "'%d-%d'\n" : "'%d%.d'\n", 1, 2);
>> return 0;
>> }
>>
>> I get
>>
>> '12'
>> '1-2'
>>
>> But maybe there *is* such a format character, it just isn't "%.d".
>
> You're right, it looks like it has to be %.s, it doesn't work with %.d.
> If you used %.s you'd probably just get a type mismatch warning instead
> :-(. There's nothing wrong with the way you wrote it, but I like to
> enable all possible warnings and eliminate them. In this case I think
> I'd just print the rid even if it's 0.
The thing is that I'd rather get rid of the RID altogether at some point,
so I wouldn't want to manifest it by printing it if it is 0 ;-).
>>>> Can you suggest a different way of causing a segfault at this point?
>>>
>>> You could add volatile as the warning suggests. Is there a good reason
>>> not to use abort() instead?
>>
>> The idea behind this was to allow for easy backtracking in such a case.
>> I believe abort() wouldn't allow this, would it?
>
> abort() does usually generate a useful core dump/stack backtrace IME.
You're right, I've changed it.
Klaus
Dear Klaus,
thanks a lot for your quick reaction and for addressing the issues!
Am Samstag, den 08.02.2014, 13:51 +0100 schrieb Klaus Schmidinger:
> On 08.02.2014 09:59, Paul Menzel wrote:
> > building VDR 2.1.3 with Clang 3.4.1 the warnings below are shown.
> >
> > Most warnings look like they can be ignored. Maybe you can spot
> > something, which should be fixed.
> >
> > $ clang --version
> > Debian clang version 3.4-1 (tags/RELEASE_34/final) (based on LLVM 3.4)
> > Target: i386-pc-linux-gnu
> > Thread model: posix
> > $ CC=clang CXX=clang++ make
> > […]
> > clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated
>
> You may want to find an option that suppresses this warning, because the file names of VDR (*.c)
> are not going to change.
I know your point of view on that one from the same issue with Cppcheck,
so I did not comment that warning. ;-)
Looking at the manual of Clang, the option below looks promising.
-ObjC++
Treat source input files as Objective-C++ inputs.
[…]
Thanks,
Paul
PS: Also big thanks to Tony for helping addressing the warnings!
===================================================================
RCS file: ./PLUGINS/src/dvbhddevice/RCS/hdffosd.c
retrieving revision 3.0
@@ -42,7 +42,6 @@
tFont mFonts[MAX_NUM_FONTS];
uint32_t mBitmapPalette;
uint32_t mBitmapColors[256];
- uint32_t mBitmapNumColors;
bool mSupportsUtf8Text;
@@ -555,7 +554,6 @@
uint32_t mDisplay;
uint32_t mBitmapPalette;
uint32_t mBitmapColors[256];
- uint32_t mBitmapNumColors;
protected:
virtual void SetActive(bool On);
===================================================================
RCS file: ./PLUGINS/src/pictures/RCS/player.c
retrieving revision 3.0
@@ -211,7 +211,7 @@
eOSState cPictureControl::ProcessKey(eKeys Key)
{
- switch (Key) {
+ switch (int(Key)) {
case kUp:
case kPlay: slideShowDelay.Set();
slideShow = true;
===================================================================
RCS file: ./RCS/ci.c
retrieving revision 3.10
@@ -864,7 +864,7 @@
#pragma pack(1)
struct tTime { uint16_t mjd; uint8_t h, m, s; short offset; };
#pragma pack()
- tTime T = { mjd : htons(MJD), h : DEC2BCD(tm_gmt.tm_hour), m : DEC2BCD(tm_gmt.tm_min), s : DEC2BCD(tm_gmt.tm_sec), offset : short(htons(tm_loc.tm_gmtoff / 60)) };
+ tTime T = { .mjd = htons(MJD), .h = DEC2BCD(tm_gmt.tm_hour), .m = DEC2BCD(tm_gmt.tm_min), .s = DEC2BCD(tm_gmt.tm_sec), .offset = short(htons(tm_loc.tm_gmtoff / 60)) };
bool OldDumpTPDUDataTransfer = DumpTPDUDataTransfer;
DumpTPDUDataTransfer &= DumpDateTime;
if (DumpDateTime)
@@ -1004,7 +1004,7 @@
case DCC_SET_MMI_MODE:
if (l == 2 && *++d == MM_HIGH_LEVEL) {
struct tDisplayReply { uint8_t id; uint8_t mode; };
- tDisplayReply dr = { id : DRI_MMI_MODE_ACK, mode : MM_HIGH_LEVEL };
+ tDisplayReply dr = { .id = DRI_MMI_MODE_ACK, .mode = MM_HIGH_LEVEL };
dbgprotocol("Slot %d: ==> Display Reply (%d)\n", Tc()->CamSlot()->SlotNumber(), SessionId());
SendData(AOT_DISPLAY_REPLY, 2, (uint8_t *)&dr);
}
===================================================================
RCS file: ./RCS/dvbspu.h
retrieving revision 3.0
@@ -101,7 +101,6 @@
uint8_t *spu;
uint32_t spupts;
bool clean;
- bool ready;
bool restricted_osd;
enum spFlag { spNONE, spHIDE, spSHOW, spMENU };
===================================================================
RCS file: ./RCS/dvbsubtitle.c
retrieving revision 3.4
@@ -34,7 +34,6 @@
static bool DebugPages = DebugVerbose || DebugNormal;
static bool DebugRegions = DebugVerbose || DebugNormal;
static bool DebugObjects = DebugVerbose || DebugNormal;
-static bool DebugBitmaps = DebugVerbose || DebugNormal;
static bool DebugConverter = DebugVerbose;
static bool DebugSegments = DebugVerbose;
static bool DebugPixel = DebugVerbose;
@@ -45,7 +44,6 @@
#define dbgpages(a...) if (DebugPages) SD.WriteHtml(a)
#define dbgregions(a...) if (DebugRegions) SD.WriteHtml(a)
#define dbgobjects(a...) if (DebugObjects) SD.WriteHtml(a)
-#define dbgbitmaps(a...) if (DebugBitmaps) SD.WriteHtml(a)
#define dbgconverter(a...) if (DebugConverter) SD.WriteHtml(a)
#define dbgsegments(a...) if (DebugSegments) SD.WriteHtml(a)
#define dbgpixel(a...) if (DebugPixel) SD.WriteHtml(a)
===================================================================
RCS file: ./RCS/eit.c
retrieving revision 3.3
@@ -220,7 +220,7 @@
case SI::LinkageDescriptorTag: {
SI::LinkageDescriptor *ld = (SI::LinkageDescriptor *)d;
tChannelID linkID(Source, ld->getOriginalNetworkId(), ld->getTransportStreamId(), ld->getServiceId());
- if (ld->getLinkageType() == 0xB0) { // Premiere World
+ if (uint(ld->getLinkageType()) == 0xB0) { // Premiere World
bool hit = StartTime <= Now && Now < StartTime + Duration;
if (hit) {
char linkName[ld->privateData.getLength() + 1];
===================================================================
RCS file: ./libsi/RCS/descriptor.h
retrieving revision 3.1
@@ -489,8 +489,6 @@
StructureLoop<Language> languageLoop;
protected:
virtual void Parse();
-private:
- const descr_iso_639_language *s;
};
class PDCDescriptor : public Descriptor {
===================================================================
RCS file: ./RCS/menu.c
retrieving revision 3.18
@@ -983,7 +983,7 @@
if (!*data.file)
strcpy(data.file, data.Channel()->ShortName(true));
if (timer) {
- if (memcmp(timer, &data, sizeof(data)) != 0)
+ if (memcmp((void *)timer, &data, sizeof(data)) != 0)
*timer = data;
if (addIfConfirmed)
Timers.Add(timer);
===================================================================
RCS file: ./RCS/remux.h
retrieving revision 3.2
@@ -483,7 +483,6 @@
bool independentFrame;
uint32_t ptsValues[MaxPtsValues]; // 32 bit is enough - we only need the delta
int numPtsValues;
- int numFrames;
int numIFrames;
bool isVideo;
double framesPerSecond;