Re: CVS commit: src/usr.bin/gzip
Date:Tue, 12 Jun 2018 19:52:43 +0200 From:Kamil Rytarowski Message-ID: <90519caf-289c-b4d3-7ebc-d14d4efc9...@gmx.com> | I will try to meet the expectations in commit messages for *San fixes. That would be good (and not just those, unless the board required it, which I doubt is likely, there's no need for the "Sponsored by" stuff almost anywhere, maybe just in those monthly reports you send). | I will try to also dig down why some programs needs in bit mask the sign | bit in unsigned integer instead of quickly swapping the type to unsigned. It is probably important to verify that making the vars unsigned will not break anything (normally for vars using bit fields and masking it will not) as breaking working code in the name of avoiding technical unspecified behaviour is not really a good idea. Fixing it is a good idea provided that the fixed version works (which it will for the cases you have changed that I have looked at I believe) but it is important that it really be a fix. | There are just many reports so I expected to land patches quickly, just | please be aware that presenting more deep investigation in a commit | message will take longer. That isn't needed either - if a program is doing something that is undefined all the commit message needs to say is something like "Avoid unspecified behaviour, found by XXXsan" That's it. If someone wants to see what the unepcified behaviour is, then they can check what lines were changed, if they can't work out why that code was not correct, they can ask (but I dount that is going to happen often, most people here recognise what C requires in this area - even if it is a bit stupid (IMO) and might not really be required for posix systems which require that int types be 2's complement, unlike what C wants to be able to run on.) kre
Re: CVS commit: src/usr.bin/gzip
On 12.06.2018 15:52, Joerg Sonnenberger wrote: > On Tue, Jun 12, 2018 at 02:35:40PM +0200, Kamil Rytarowski wrote: >> On 12.06.2018 14:24, Joerg Sonnenberger wrote: >>> That's missing the point. A short description of why the specific >>> undefined behavior is seen is useful. Pasting random program output is >>> not. >>> >>> Joerg >>> >> >> Random program output might not be useful, but the one containing >> runtime message is.. and it was 1-liner + 1 line how to invoke it and 1 >> line of runtime error. I have more to come an I will keep documenting >> the runtime error messages. > > Are you trying hard to be obnoxious? The only useful part that you > quoted is the runtime error itself. The rest is either noise or doesn't > help with reproduction since it depends on external input. Nothing in > the commit message contains a real analysis of why this "reproducer" > triggers the problem. As such, it is pointless noise and shouldn't be > preserved for eternity. > > Joerg > I will try to meet the expectations in commit messages for *San fixes. I will try to also dig down why some programs needs in bit mask the sign bit in unsigned integer instead of quickly swapping the type to unsigned. There are just many reports so I expected to land patches quickly, just please be aware that presenting more deep investigation in a commit message will take longer. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/bin/ksh
On Tue, Jun 12, 2018 at 14:13:55 +, Kamil Rytarowski wrote: > UBSan can detect that during switching a login to root there is unportable > left shift operation: > > $ su - > Password: > /public/src.git/bin/ksh/eval.c:598:13: runtime error: left shift of > 1073741824 by 1 places cannot be represented in type 'int' > # Is this, seriously, how you reproduce it? It doesn't trigger on just running ksh? In which case what is the real problem here - it must be triggered by some .profile code, which your "reproducer" doesn't provide. -uwe
Re: CVS commit: src/usr.bin/gzip
On Tue, Jun 12, 2018 at 02:35:40PM +0200, Kamil Rytarowski wrote: > On 12.06.2018 14:24, Joerg Sonnenberger wrote: > > That's missing the point. A short description of why the specific > > undefined behavior is seen is useful. Pasting random program output is > > not. > > > > Joerg > > > > Random program output might not be useful, but the one containing > runtime message is.. and it was 1-liner + 1 line how to invoke it and 1 > line of runtime error. I have more to come an I will keep documenting > the runtime error messages. Are you trying hard to be obnoxious? The only useful part that you quoted is the runtime error itself. The rest is either noise or doesn't help with reproduction since it depends on external input. Nothing in the commit message contains a real analysis of why this "reproducer" triggers the problem. As such, it is pointless noise and shouldn't be preserved for eternity. Joerg
Re: CVS commit: src/usr.bin/gzip
On 12.06.2018 14:24, Joerg Sonnenberger wrote: > On Tue, Jun 12, 2018 at 12:00:01PM +0200, Kamil Rytarowski wrote: >> On 12.06.2018 11:51, matthew green wrote: On 12.06.2018 10:28, Kamil Rytarowski wrote: > On 12.06.2018 09:04, Martin Husemann wrote: >> On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: >>> To sum it up, out of 30+ lines of the commit message, the relevant >>> information is contained only in (part of) one line. >> >> FWIW, I fully agree with uwe here. >> >> Martin > > I find keeping reproducers for issues very useful. Keeping track of them > helps to check whether fixes are functional. > > Also introduction of refactoring without a note in the message is not > acceptable in my opinion. > > Thanks to the verbose message people have the whole context. To be clear, I will keep introducing fixes in the same form. I'm catching e.g. bugs in programs only in specific usage and input. If I will refactor something I will keep including it in messages too. >>> >>> that's a pity. >>> >>> i don't mind having a little more detail that uwe is talking >>> about, but i don't think we need nearly as much. it's worth >>> mentioning the sanitizer used as the finding-tool, but there >>> is no need to repeat the basic fix 3 times, or to reproduce >>> the code change itself. >>> >>> please reconsider and use a shorter form. >>> >> >> I will keep messages within 20 lines. > > That's missing the point. A short description of why the specific > undefined behavior is seen is useful. Pasting random program output is > not. > > Joerg > Random program output might not be useful, but the one containing runtime message is.. and it was 1-liner + 1 line how to invoke it and 1 line of runtime error. I have more to come an I will keep documenting the runtime error messages. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/usr.bin/gzip
On 12.06.2018 11:51, matthew green wrote: >> On 12.06.2018 10:28, Kamil Rytarowski wrote: >>> On 12.06.2018 09:04, Martin Husemann wrote: On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: > To sum it up, out of 30+ lines of the commit message, the relevant > information is contained only in (part of) one line. FWIW, I fully agree with uwe here. Martin >>> >>> I find keeping reproducers for issues very useful. Keeping track of them >>> helps to check whether fixes are functional. >>> >>> Also introduction of refactoring without a note in the message is not >>> acceptable in my opinion. >>> >>> Thanks to the verbose message people have the whole context. >> >> To be clear, I will keep introducing fixes in the same form. I'm >> catching e.g. bugs in programs only in specific usage and input. If I >> will refactor something I will keep including it in messages too. > > that's a pity. > > i don't mind having a little more detail that uwe is talking > about, but i don't think we need nearly as much. it's worth > mentioning the sanitizer used as the finding-tool, but there > is no need to repeat the basic fix 3 times, or to reproduce > the code change itself. > > please reconsider and use a shorter form. > I will keep messages within 20 lines. signature.asc Description: OpenPGP digital signature
re: CVS commit: src/usr.bin/gzip
>On 12.06.2018 10:28, Kamil Rytarowski wrote: >> On 12.06.2018 09:04, Martin Husemann wrote: >>> On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: To sum it up, out of 30+ lines of the commit message, the relevant information is contained only in (part of) one line. >>> >>> FWIW, I fully agree with uwe here. >>> >>> Martin >> >> I find keeping reproducers for issues very useful. Keeping track of them >> helps to check whether fixes are functional. >> >> Also introduction of refactoring without a note in the message is not >> acceptable in my opinion. >> >> Thanks to the verbose message people have the whole context. > >To be clear, I will keep introducing fixes in the same form. I'm >catching e.g. bugs in programs only in specific usage and input. If I >will refactor something I will keep including it in messages too. that's a pity. i don't mind having a little more detail that uwe is talking about, but i don't think we need nearly as much. it's worth mentioning the sanitizer used as the finding-tool, but there is no need to repeat the basic fix 3 times, or to reproduce the code change itself. please reconsider and use a shorter form. .mrg.
Re: CVS commit: src/usr.bin/gzip
On 12.06.2018 10:28, Kamil Rytarowski wrote: > On 12.06.2018 09:04, Martin Husemann wrote: >> On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: >>> To sum it up, out of 30+ lines of the commit message, the relevant >>> information is contained only in (part of) one line. >> >> FWIW, I fully agree with uwe here. >> >> Martin >> > > I find keeping reproducers for issues very useful. Keeping track of them > helps to check whether fixes are functional. > > Also introduction of refactoring without a note in the message is not > acceptable in my opinion. > > Thanks to the verbose message people have the whole context. > To be clear, I will keep introducing fixes in the same form. I'm catching e.g. bugs in programs only in specific usage and input. If I will refactor something I will keep including it in messages too. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/usr.bin/gzip
On 12.06.2018 09:04, Martin Husemann wrote: > On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: >> To sum it up, out of 30+ lines of the commit message, the relevant >> information is contained only in (part of) one line. > > FWIW, I fully agree with uwe here. > > Martin > I find keeping reproducers for issues very useful. Keeping track of them helps to check whether fixes are functional. Also introduction of refactoring without a note in the message is not acceptable in my opinion. Thanks to the verbose message people have the whole context. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/usr.bin/gzip
On Tue, Jun 12, 2018 at 05:47:35AM +0300, Valery Ushakov wrote: > To sum it up, out of 30+ lines of the commit message, the relevant > information is contained only in (part of) one line. FWIW, I fully agree with uwe here. Martin
