Re: CVS commit: src/tests/lib/libc/sys
In article <20200213114904.ga30...@bec.de>, Joerg Sonnenberger wrote: >On Thu, Feb 13, 2020 at 10:50:19AM +0100, Joerg Sonnenberger wrote: >> On Wed, Feb 12, 2020 at 09:53:46PM -0500, Christos Zoulas wrote: >> > Module Name: src >> > Committed By: christos >> > Date: Thu Feb 13 02:53:46 UTC 2020 >> > >> > Modified Files: >> >src/tests/lib/libc/sys: t_ptrace_x86_wait.h >> > >> > Log Message: >> > Turn off optimization on a function which contains constant labels. >> > The optimizer splits it and we end up with 2 copies and duplicate symbols. >> >> Why not just turn them into local labels instead? > >Alternatively, suffixing them with %= would create unique labels. I was looking for that :-) christos
Re: CVS commit: src/tests/lib/libc/sys
In article <20200213095019.ga28...@bec.de>, Joerg Sonnenberger wrote: >On Wed, Feb 12, 2020 at 09:53:46PM -0500, Christos Zoulas wrote: >> Module Name: src >> Committed By:christos >> Date:Thu Feb 13 02:53:46 UTC 2020 >> >> Modified Files: >> src/tests/lib/libc/sys: t_ptrace_x86_wait.h >> >> Log Message: >> Turn off optimization on a function which contains constant labels. >> The optimizer splits it and we end up with 2 copies and duplicate symbols. > >Why not just turn them into local labels instead? You mean 1f etc? I was not sure if that would work in the symbol defined case. christos
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 4:32 PM Kamil Rytarowski wrote: > > On 13.02.2020 22:20, Valery Ushakov wrote: > > I did not propose to disable the warning. I proposed to downgrade > > -Werror to -Wno-error (i.e. a warning) and only for the buggy > > sanitizer build. That file will still be compiled in normal builds > > with all the warnings=errors enabled, so real problems won't be > > overlooked. > > OK, we can try this path. > > Santosh, could you please revert and try -Wno-error + upstream it? > > Thank you in advance! > Sure, let me prepare the patch. -- Santhosh
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On 13.02.2020 22:20, Valery Ushakov wrote: > I did not propose to disable the warning. I proposed to downgrade > -Werror to -Wno-error (i.e. a warning) and only for the buggy > sanitizer build. That file will still be compiled in normal builds > with all the warnings=errors enabled, so real problems won't be > overlooked. OK, we can try this path. Santosh, could you please revert and try -Wno-error + upstream it? Thank you in advance! signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 18:25:41 +0100, Kamil Rytarowski wrote: > On 13.02.2020 18:00, Valery Ushakov wrote: > > Arguably, if the tool you use is broken, you shouln't be mutilating > > the source code just to shut that tool up. > > The introduced changes were good, even if GCC would be silent. You changed one occurence just because it happens to trigger a bug in gcc. There are ntohs() >< size_t_var comparisons right above and below the line you changed, where the same promotion happens (without triggering a gcc bug) and they don't have a cast. So someone reading that code will ask themself, wait a minute, why the cast is necessary in *that* place but not in those places? What is going on there that I miss that required introduction of that cast. I.e. your change create cognitive load for the reader. I don't cosider that good. > [...] the promotion rules are considered by many people as the major > flaw in C. Today I prefer explicit casts (after the MUSL lesson) > even if unnecessary than implicit promotion. Amen. However they are there and we made uneasy peace with them so unless you are consistent in your casting, you send all the wrong singals to the reader. > As an alternative option we can disable warnings but this is in my > opinion much worse in this case than potentially overlooking real > problems in a 4000+ line file. I did not propose to disable the warning. I proposed to downgrade -Werror to -Wno-error (i.e. a warning) and only for the buggy sanitizer build. That file will still be compiled in normal builds with all the warnings=errors enabled, so real problems won't be overlooked. > Repeatedly informing that the tool (GCC) is broken did not solve any > issue. It would be finally better to see someone fixing GCC rather > than informing other GCC users about it. Feel free to follow your own advice here... (After all, it's your sanitizer usage that has problems. The normal builds are fine as they are :) > On the opposite side of this if this camp is MUSL. I used chunks of the > MUSL code and it had poor results. There is a strict policy to avoid > casts unless absolutely needed That's not a bad policy. As I said, a cast is a blunt tool. It's easy to introduce a wrong one that would silence some warning or other but will do the wrong thing, but now since there is a cast the compiler cannot even squeak. I might be misremembering, but IIRC Visual C has some warning(s) that ignore explicit casts, precisely to detect that kind of problems. > and if they are needed this tends to be in as obscure way as > possible (like adding U attribute to one of the arguments in an > expression). What's obscure about adding 'U' to signify unsignedness?! Also it's not a cast to begin with, a literal with the 'u' suffix *is* unsigned. A cast is when you take something of one type and coerce it to be of some other type. -uwe
Re: CVS commit: src/sys/external/bsd/drm2/dist/drm/nouveau
The static variable was in #ifdef __NetBSD__ part, so I assumed it doesn't influence the merge. Christos already reverted the fallthrough warning fix. Jaromir Le jeu. 13 févr. 2020 à 09:18, matthew green a écrit : > > "Jaromir Dolecek" writes: > > Module Name: src > > Committed By: jdolecek > > Date: Wed Feb 12 20:31:46 UTC 2020 > > > > Modified Files: > > src/sys/external/bsd/drm2/dist/drm/nouveau: nouveau_fbcon.c > > > > Log Message: > > remove superfluous static variable used only to zero attach args > > is this necessary for 3rd party code? > > please try to avoid changing these for cosmetic reasons. > > same for warned code that is forced to warning not error. > the reason we/i didn't change them is to avoid making > future drm updates harder -- they're already *really* hard. > > thanks. > > > .mrg.
Re: CVS commit: src/lib/libpthread
Hi,
Kamil Rytarowski writes:
> On 12.02.2020 15:01, Ryo ONODERA wrote:
>> Hi,
>>
>> Kamil Rytarowski writes:
>>
>>> Hello,
>>>
>>> I will have a look at them.
>>
>> Thank you.
>> Real fix is welcome.
>>
>> And multimedia/handbrake has workaround already.
>> I have workaround patches for lang/mono6 (like your nspr patch).
>> I will commit them after some tests.
>>
>
> libblueray real fix patch is pending upstream.
>
> https://code.videolan.org/videolan/libbluray/merge_requests/17
Thank you very much!
I will apply this to multimedia/handbrake too.
> I will look into mono next.
Excellent.
>
>>> On 12.02.2020 14:02, Ryo ONODERA wrote:
Hi,
Kamil Rytarowski writes:
> Please apple workaround (same like in NSPR) for now if fixing is
> difficult.
>
> Such bugs can have security implications.
Adding workarounds will not improve security problems.
And I feel that such workarounds will not be accepted by upstream.
I will add workarounds to some packages.
However I feel that it is not meaningful...
> On 12.02.2020 09:49, Ryo ONODERA wrote:
>> Hi,
>>
>> I have two problematic pkgsrc packages at least.
>> Of course these programs have misuses and/or bugs, however I feel that
>> dealing pt_magic in pthread_equal() is too hasty for pkgsrc.
>>
>> multimedia/handbrake (internal libbluray):
>> The invalid thread pointer is not NULL.
>> pthread_equal t1: 0x
>> pthread_equal t2: 0x7073b25e2000
>>
>> Another one is lang/mono6:
>> The invalid thread pointer is not 0x.
>> pthread_equal t1: 0x7b066d4d7800
>> pthread_equal t2: 0x60f5f000
>>
>> Of course, it is desirable to fix every misuses and bugs in pkgsrc.
>> However it is impossible for now (at least for me).
>>
>> "Kamil Rytarowski" writes:
>>
>>> Module Name:src
>>> Committed By: kamil
>>> Date: Sat Feb 8 17:06:03 UTC 2020
>>>
>>> Modified Files:
>>> src/lib/libpthread: pthread.c
>>>
>>> Log Message:
>>> Change the behavior of pthread_equal()
>>>
>>> On error when not aborting, do not return EINVAL as it has a side effect
>>> of being interpreted as matching threads. For invalid threads return
>>> unmatched.
>>>
>>> Check pthreads for NULL, before accessing pt_magic field. This avoids
>>> faults on comparision with a NULL pointer.
>>>
>>> This behavior is in the scope of UB, but should be easier to deal with
>>> buggy software.
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.163 -r1.164 src/lib/libpthread/pthread.c
>>>
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>>
>>> Modified files:
>>>
>>> Index: src/lib/libpthread/pthread.c
>>> diff -u src/lib/libpthread/pthread.c:1.163
>>> src/lib/libpthread/pthread.c:1.164
>>> --- src/lib/libpthread/pthread.c:1.163 Wed Feb 5 14:56:04 2020
>>> +++ src/lib/libpthread/pthread.cSat Feb 8 17:06:03 2020
>>> @@ -1,4 +1,4 @@
>>> -/* $NetBSD: pthread.c,v 1.163 2020/02/05 14:56:04 ryoon Exp $
>>> */
>>> +/* $NetBSD: pthread.c,v 1.164 2020/02/08 17:06:03 kamil Exp $
>>> */
>>>
>>> /*-
>>> * Copyright (c) 2001, 2002, 2003, 2006, 2007, 2008, 2020
>>> @@ -31,7 +31,7 @@
>>> */
>>>
>>> #include
>>> -__RCSID("$NetBSD: pthread.c,v 1.163 2020/02/05 14:56:04 ryoon Exp $");
>>> +__RCSID("$NetBSD: pthread.c,v 1.164 2020/02/08 17:06:03 kamil Exp $");
>>>
>>> #define__EXPOSE_STACK 1
>>>
>>> @@ -770,11 +770,11 @@ pthread_equal(pthread_t t1, pthread_t t2
>>> if (__predict_false(__uselibcstub))
>>> return __libc_thr_equal_stub(t1, t2);
>>>
>>> - pthread__error(EINVAL, "Invalid thread",
>>> - t1->pt_magic == PT_MAGIC);
>>> + pthread__error(0, "Invalid thread",
>>> + (t1 != NULL) && (t1->pt_magic == PT_MAGIC));
>>>
>>> - pthread__error(EINVAL, "Invalid thread",
>>> - t2->pt_magic == PT_MAGIC);
>>> + pthread__error(0, "Invalid thread",
>>> + (t2 != NULL) && (t2->pt_magic == PT_MAGIC));
>>>
>>> /* Nothing special here. */
>>> return (t1 == t2);
>>>
>>
>
>
>>>
>>>
>>
>
>
--
Ryo ONODERA // r...@tetera.org
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On 13.02.2020 18:00, Valery Ushakov wrote: > Arguably, if the tool you use is broken, you shouln't be mutilating > the source code just to shut that tool up. The introduced changes were good, even if GCC would be silent. It is far from mutilating. As an alternative option we can disable warnings but this is in my opinion much worse in this case than potentially overlooking real problems in a 4000+ line file. On the opposite side of this if this camp is MUSL. I used chunks of the MUSL code and it had poor results. There is a strict policy to avoid casts unless absolutely needed and if they are needed this tends to be in as obscure way as possible (like adding U attribute to one of the arguments in an expression). This resulted in unmaintainable code and very difficult situation to guess whether code semantics is broken or buggy. For purists, MUSL is not mutilated at all so people wanting this style are informed where to find it now. Today I prefer explicit casts (after the MUSL lesson) even if unnecessary than implicit promotion. I'm not alone in here as the promotion rules are considered by many people as the major flaw in C. Everybody agrees that GCC was picky in that case and not mandatory from a strict language point of view. Repeatedly informing that the tool (GCC) is broken did not solve any issue. It would be finally better to see someone fixing GCC rather than informing other GCC users about it. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/libpthread
On 12.02.2020 15:01, Ryo ONODERA wrote:
> Hi,
>
> Kamil Rytarowski writes:
>
>> Hello,
>>
>> I will have a look at them.
>
> Thank you.
> Real fix is welcome.
>
> And multimedia/handbrake has workaround already.
> I have workaround patches for lang/mono6 (like your nspr patch).
> I will commit them after some tests.
>
libblueray real fix patch is pending upstream.
https://code.videolan.org/videolan/libbluray/merge_requests/17
I will look into mono next.
>> On 12.02.2020 14:02, Ryo ONODERA wrote:
>>> Hi,
>>>
>>> Kamil Rytarowski writes:
>>>
Please apple workaround (same like in NSPR) for now if fixing is difficult.
Such bugs can have security implications.
>>>
>>> Adding workarounds will not improve security problems.
>>> And I feel that such workarounds will not be accepted by upstream.
>>> I will add workarounds to some packages.
>>> However I feel that it is not meaningful...
>>>
On 12.02.2020 09:49, Ryo ONODERA wrote:
> Hi,
>
> I have two problematic pkgsrc packages at least.
> Of course these programs have misuses and/or bugs, however I feel that
> dealing pt_magic in pthread_equal() is too hasty for pkgsrc.
>
> multimedia/handbrake (internal libbluray):
> The invalid thread pointer is not NULL.
> pthread_equal t1: 0x
> pthread_equal t2: 0x7073b25e2000
>
> Another one is lang/mono6:
> The invalid thread pointer is not 0x.
> pthread_equal t1: 0x7b066d4d7800
> pthread_equal t2: 0x60f5f000
>
> Of course, it is desirable to fix every misuses and bugs in pkgsrc.
> However it is impossible for now (at least for me).
>
> "Kamil Rytarowski" writes:
>
>> Module Name: src
>> Committed By:kamil
>> Date:Sat Feb 8 17:06:03 UTC 2020
>>
>> Modified Files:
>> src/lib/libpthread: pthread.c
>>
>> Log Message:
>> Change the behavior of pthread_equal()
>>
>> On error when not aborting, do not return EINVAL as it has a side effect
>> of being interpreted as matching threads. For invalid threads return
>> unmatched.
>>
>> Check pthreads for NULL, before accessing pt_magic field. This avoids
>> faults on comparision with a NULL pointer.
>>
>> This behavior is in the scope of UB, but should be easier to deal with
>> buggy software.
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.163 -r1.164 src/lib/libpthread/pthread.c
>>
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>>
>> Modified files:
>>
>> Index: src/lib/libpthread/pthread.c
>> diff -u src/lib/libpthread/pthread.c:1.163
>> src/lib/libpthread/pthread.c:1.164
>> --- src/lib/libpthread/pthread.c:1.163 Wed Feb 5 14:56:04 2020
>> +++ src/lib/libpthread/pthread.c Sat Feb 8 17:06:03 2020
>> @@ -1,4 +1,4 @@
>> -/* $NetBSD: pthread.c,v 1.163 2020/02/05 14:56:04 ryoon Exp $
>> */
>> +/* $NetBSD: pthread.c,v 1.164 2020/02/08 17:06:03 kamil Exp $
>> */
>>
>> /*-
>> * Copyright (c) 2001, 2002, 2003, 2006, 2007, 2008, 2020
>> @@ -31,7 +31,7 @@
>> */
>>
>> #include
>> -__RCSID("$NetBSD: pthread.c,v 1.163 2020/02/05 14:56:04 ryoon Exp $");
>> +__RCSID("$NetBSD: pthread.c,v 1.164 2020/02/08 17:06:03 kamil Exp $");
>>
>> #define __EXPOSE_STACK 1
>>
>> @@ -770,11 +770,11 @@ pthread_equal(pthread_t t1, pthread_t t2
>> if (__predict_false(__uselibcstub))
>> return __libc_thr_equal_stub(t1, t2);
>>
>> -pthread__error(EINVAL, "Invalid thread",
>> -t1->pt_magic == PT_MAGIC);
>> +pthread__error(0, "Invalid thread",
>> +(t1 != NULL) && (t1->pt_magic == PT_MAGIC));
>>
>> -pthread__error(EINVAL, "Invalid thread",
>> -t2->pt_magic == PT_MAGIC);
>> +pthread__error(0, "Invalid thread",
>> +(t2 != NULL) && (t2->pt_magic == PT_MAGIC));
>>
>> /* Nothing special here. */
>> return (t1 == t2);
>>
>
>>>
>>
>>
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 15:03:35 +0100, Kamil Rytarowski wrote: > On 13.02.2020 14:50, Valery Ushakov wrote: > > On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote: > > > > Can you show the original problem that you are trying to fix here? > > When and how did you get warning? > > GCC has a property (as called by Joerg bug) that different backend, > different optimization levels etc emit different warnings. > > There is a request from certain people around to fix GCC not to mute > down GCC, only because warnings are emitted in one configuration and not > in their build settings. > > I redirect these complains to GCC upstream. > > dhcpcd emits these warnings once we enable -fsanitize=undefined. If I add -fsanitize=undefined to the command line to compile that file (in current) I still do not see that warning. I didn't have time yet to do a full build with sanitizer enabled. Arguably, if the tool you use is broken, you shouln't be mutilating the source code just to shut that tool up. In general I'm quite worried about the recent ongoing activity of sprinkling changes around the tree just to shut up this or that warning without paying much attention to the end result. Some of those changes were wrong. Some left code looking inconsistent as they fixed one place that did produce a warning, but didn't change nearly identical bits of code just next to it that didn't. That leaves code in a fractured state. Casts in particular are a very blunt instrument, and given that history of recent changes your use of that instrument in this case really raised red flags. If -- and I still haven't seen it myself, "works for me" in a naive test -- -fsanitize=undefined is buggy w.r.t. -Wconversion, then you should arange to use say -Wno-error=conversion for that specific file when -fsanitize=undefined is enabled, we do have makefile machinery for that. This way the bug in the tool is not causing random and dubious changes to the code, and is confined to a properly commented change in a Makefile. -uwe
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On 13.02.2020 14:50, Valery Ushakov wrote: > On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote: > >> > Can you show the original problem that you are trying to fix here? > When and how did you get warning? > GCC has a property (as called by Joerg bug) that different backend, different optimization levels etc emit different warnings. There is a request from certain people around to fix GCC not to mute down GCC, only because warnings are emitted in one configuration and not in their build settings. I redirect these complains to GCC upstream. dhcpcd emits these warnings once we enable -fsanitize=undefined. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 14:18:43 +0100, Kamil Rytarowski wrote:
> Feel free to fix GCC.
That's sounds rather dismissive to me.
Can you show the original problem that you are trying to fix here?
When and how did you get warning?
I do NOT see the warning for this kind of conversion with either
current (gcc 8) or on netbsd-8 (gcc 5).
If I revert this change the file is compiled just fine. E.g. with a
very current current build:
$ nbmake-sparc dhcp.o
# compile dhcpcd/dhcp.o
sparc--netbsdelf-gcc -O2 ... -Wconversion -Wsign-compare -c .../dhcp.c
$
> dhcpcd already uses this explicit cast style, e.g. here:
>
> static void *
> get_udp_data(void *packet, size_t *len)
> {
> const struct ip *ip = packet;
> size_t ip_hl = (size_t)ip->ip_hl * 4;
> char *p = packet;
>
> p += ip_hl + sizeof(struct udphdr);
> *len = (size_t)ntohs(ip->ip_len) - sizeof(struct udphdr) - ip_hl;
> return p;
> }
Again, I don't get any warnings if I change that to
... /*(size_t)*/ntohs(ip->ip_len) ...
I also tried
... /*(size_t)ntohs*/(ip->ip_len) ...
as a quick and dirty attempt to emulate little endian where ntohs is a
nop (I don't have an up-to-date little endian build handy) and, again,
there's no warning.
Oh, actually, my tooldir has accreted quite a number of old superh
compilers, so:
$ cat size.c
typedef unsigned short uint16_t;
typedef unsigned long size_t;
#define ntohs(x) (x)
size_t
foo(uint16_t ip_len)
{
return ntohs(ip_len) - sizeof(uint16_t);
}
$ for CC in $TOOLDIR/bin/shle--netbsdelf-gcc-[0-9]*; do echo $(basename $CC);
$CC -Wall -Wextra -Wconversion -Wsign-compare -O2 -S size.c; done
shle--netbsdelf-gcc-4.5.4
shle--netbsdelf-gcc-4.8.3
shle--netbsdelf-gcc-4.8.4
shle--netbsdelf-gcc-4.8.5
shle--netbsdelf-gcc-5.4.0
shle--netbsdelf-gcc-5.5.0
shle--netbsdelf-gcc-6.4.0
shle--netbsdelf-gcc-6.5.0
shle--netbsdelf-gcc-7.4.0
$
-uwe
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On 13.02.2020 14:13, Joerg Sonnenberger wrote:
> On Thu, Feb 13, 2020 at 02:05:12PM +0100, Kamil Rytarowski wrote:
>> On 13.02.2020 00:58, Joerg Sonnenberger wrote:
>>> On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote:
On 09/02/2020 19:21, Joerg Sonnenberger wrote:
> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote:
>> Module Name: src
>> Committed By:fox
>> Date:Sat Feb 8 12:17:16 UTC 2020
>>
>> Modified Files:
>> src/external/bsd/dhcpcd/dist/src: dhcp.c
>>
>> Log Message:
>> external/bsd/dhcpcd: Fix a -Wconversion warning.
>>
>> Type cast uint16_t to size_t to prevent implicit type conversion.
>
> Seriously? That should not warn and no cast should be used either.
What fix would you recommend then?
>>>
>>> Disable the warning in GCC and fill an upstream PR against it. A
>>> conversion from uint16_t to size_t is value preserving by definition of
>>> the ISO C platform limits. It should never create a warning.
>>>
>>> Joerg
>>>
>>
>> If we disable warnings in our core software in non-trivial source file
>> for over 4000 lines we will mute more serious issues in a pretty
>> sensitive file. We fixed this warning in upstream dhcpcd.
>
> The fix is IMO significantly worse as it actually creates technical
> debt. Frankly, I would go so far and say that this should be reverted.
> This warning seems to be pretty fundamentally broken by design if the
> analysis so far is correct.
>
> Joerg
>
Feel free to fix GCC.
dhcpcd already uses this explicit cast style, e.g. here:
static void *
get_udp_data(void *packet, size_t *len)
{
const struct ip *ip = packet;
size_t ip_hl = (size_t)ip->ip_hl * 4;
char *p = packet;
p += ip_hl + sizeof(struct udphdr);
*len = (size_t)ntohs(ip->ip_len) - sizeof(struct udphdr) - ip_hl;
return p;
}
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On Thu, Feb 13, 2020 at 02:05:12PM +0100, Kamil Rytarowski wrote: > On 13.02.2020 00:58, Joerg Sonnenberger wrote: > > On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote: > >> On 09/02/2020 19:21, Joerg Sonnenberger wrote: > >>> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote: > Module Name: src > Committed By:fox > Date:Sat Feb 8 12:17:16 UTC 2020 > > Modified Files: > src/external/bsd/dhcpcd/dist/src: dhcp.c > > Log Message: > external/bsd/dhcpcd: Fix a -Wconversion warning. > > Type cast uint16_t to size_t to prevent implicit type conversion. > >>> > >>> Seriously? That should not warn and no cast should be used either. > >> > >> What fix would you recommend then? > > > > Disable the warning in GCC and fill an upstream PR against it. A > > conversion from uint16_t to size_t is value preserving by definition of > > the ISO C platform limits. It should never create a warning. > > > > Joerg > > > > If we disable warnings in our core software in non-trivial source file > for over 4000 lines we will mute more serious issues in a pretty > sensitive file. We fixed this warning in upstream dhcpcd. The fix is IMO significantly worse as it actually creates technical debt. Frankly, I would go so far and say that this should be reverted. This warning seems to be pretty fundamentally broken by design if the analysis so far is correct. Joerg
Re: CVS commit: src/external/bsd/dhcpcd/dist/src
On 13.02.2020 00:58, Joerg Sonnenberger wrote: > On Mon, Feb 10, 2020 at 04:45:35PM +, Roy Marples wrote: >> On 09/02/2020 19:21, Joerg Sonnenberger wrote: >>> On Sat, Feb 08, 2020 at 12:17:16PM +, Santhosh Raju wrote: Module Name: src Committed By: fox Date: Sat Feb 8 12:17:16 UTC 2020 Modified Files: src/external/bsd/dhcpcd/dist/src: dhcp.c Log Message: external/bsd/dhcpcd: Fix a -Wconversion warning. Type cast uint16_t to size_t to prevent implicit type conversion. >>> >>> Seriously? That should not warn and no cast should be used either. >> >> What fix would you recommend then? > > Disable the warning in GCC and fill an upstream PR against it. A > conversion from uint16_t to size_t is value preserving by definition of > the ISO C platform limits. It should never create a warning. > > Joerg > If we disable warnings in our core software in non-trivial source file for over 4000 lines we will mute more serious issues in a pretty sensitive file. We fixed this warning in upstream dhcpcd. We ship with GCC that is typically 2 major versions behind upstream GCC and this at least discourages handling generic issues directly with upstream and upstream has little interest in reiterating over old branches for corner cases that are generally speaking unimportant. So overall I disagree with the request. Feel free to contribute a patch to GCC or find someone in GCC to fix it (we have no resources to do it on our side) so we can remove the cast. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On Thu, Feb 13, 2020 at 10:50:19AM +0100, Joerg Sonnenberger wrote: > On Wed, Feb 12, 2020 at 09:53:46PM -0500, Christos Zoulas wrote: > > Module Name:src > > Committed By: christos > > Date: Thu Feb 13 02:53:46 UTC 2020 > > > > Modified Files: > > src/tests/lib/libc/sys: t_ptrace_x86_wait.h > > > > Log Message: > > Turn off optimization on a function which contains constant labels. > > The optimizer splits it and we end up with 2 copies and duplicate symbols. > > Why not just turn them into local labels instead? Alternatively, suffixing them with %= would create unique labels. Joerg
Re: CVS commit: src/tests/lib/libc/sys
On Wed, Feb 12, 2020 at 09:53:46PM -0500, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Thu Feb 13 02:53:46 UTC 2020 > > Modified Files: > src/tests/lib/libc/sys: t_ptrace_x86_wait.h > > Log Message: > Turn off optimization on a function which contains constant labels. > The optimizer splits it and we end up with 2 copies and duplicate symbols. Why not just turn them into local labels instead? Joerg
re: CVS commit: src/sys/external/bsd/drm2/dist/drm/nouveau
"Jaromir Dolecek" writes: > Module Name: src > Committed By: jdolecek > Date: Wed Feb 12 20:31:46 UTC 2020 > > Modified Files: > src/sys/external/bsd/drm2/dist/drm/nouveau: nouveau_fbcon.c > > Log Message: > remove superfluous static variable used only to zero attach args is this necessary for 3rd party code? please try to avoid changing these for cosmetic reasons. same for warned code that is forced to warning not error. the reason we/i didn't change them is to avoid making future drm updates harder -- they're already *really* hard. thanks. .mrg.
