| Message ID | 20141009221154.GA24774@amd |
|---|---|
| State | Not Applicable |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Pavel, In message <20141009221154.GA24774@amd> you wrote: > > Something like this could help..? > Pavel > > --- /dev/null 2014-10-09 01:15:57.354292026 +0200 > +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 Is there anything wrong with [1] ? [1] http://www.denx.de/wiki/U-Boot/Patches Best regards, Wolfgang Denk
Hi! > > Something like this could help..? > > Pavel > > > > --- /dev/null 2014-10-09 01:15:57.354292026 +0200 > > +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 > > Is there anything wrong with [1] ? > > [1] http://www.denx.de/wiki/U-Boot/Patches It should really go into tree. It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy. Pavel
On Fri 2014-10-10 00:24:46, Wolfgang Denk wrote: > Dear Pavel, > > In message <20141009221154.GA24774@amd> you wrote: > > > > Something like this could help..? > > Pavel > > > > --- /dev/null 2014-10-09 01:15:57.354292026 +0200 > > +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 > > Is there anything wrong with [1] ? > > [1] http://www.denx.de/wiki/U-Boot/Patches ..and actually... it makes submitting patches rather hard. [PATCH] fix compilation on socfpga > Please add tags to the subject [PATCHv2] arm: socfpga: fix compilation on socfpga > Please add diff from previous version [PATCHv3] arm: socfpga: fix compilation on socfpga --- v2: added tags to the subject v3: added diffs to previous version . (From memory, but IIRC something very similar to this happened before). This scares of all but the most determined patch submitters, and does not really improve code quality. I'd argue that if only changelog is updated, it is _not_ a new version of patch, and does not need changelog diff. Or maybe be less strict policy / less strict enforcement of the policy in trivial cases. Best regards, Pavel
On Fri, Oct 10, 2014 at 12:11:54AM +0200, Pavel Machek wrote: > Hi! > > > I don't this Albert is the problem, I am starting to suspect we simply lack > > custodian manpower in general. And I also suspect we're not quite inviting > > and attractive crowd, which is something we should discuss too ... > > As I said privately, I believe we have way too many custodians... I think perhaps I need to better spell out custodian expectations. But as to having too many? No, I need some convincing in that direction. > Anyway, u-boot code looks similar to kernel code, but patch submission > rules are really different. > > Something like this could help..? > Pavel > > --- /dev/null 2014-10-09 01:15:57.354292026 +0200 > +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 > @@ -0,0 +1,11 @@ > +Differences from kernel: Perhaps. If not in tree, a concice reminder to follow all of the rules spelled out on the wiki to avoid the ping-ponging you noted in a follow up later.
Hi Pavel, On Fri, 10 Oct 2014 01:05:59 +0200, Pavel Machek <pavel@denx.de> wrote: > On Fri 2014-10-10 00:24:46, Wolfgang Denk wrote: > > Dear Pavel, > > > > In message <20141009221154.GA24774@amd> you wrote: > > > > > > Something like this could help..? > > > Pavel > > > > > > --- /dev/null 2014-10-09 01:15:57.354292026 +0200 > > > +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 > > > > Is there anything wrong with [1] ? > > > > [1] http://www.denx.de/wiki/U-Boot/Patches > > ..and actually... it makes submitting patches rather hard. > > [PATCH] fix compilation on socfpga > > > Please add tags to the subject > > [PATCHv2] arm: socfpga: fix compilation on socfpga > > > Please add diff from previous version > > [PATCHv3] arm: socfpga: fix compilation on socfpga > > --- > > v2: added tags to the subject Tags can be useful in automating CC: lists from Patman through doc/git-mailrc, and as a filtering key in e.g. gitk, hence the suggestion to add them. Guessing which tags a patch could use is indeed a tedious and uncertain process, but I don't think it is requested of many patches, is it? > v3: added diffs to previous version > > . (From memory, but IIRC something very similar to this happened before). At least it happened that I requested the change logs when they were missing entirely in a v2-or-later series. The reason is that with these logs, reviewers can see what change requests were acknowledged by the submitter and what other changes were spontaneous additions. > This scares of all but the most determined patch submitters, and does > not really improve code quality. One can argue that it improves code /review/, by both making sure the submitter has involved the relevant custodians (tags) and provided a follow-up on their previous remarks (diffs). Note that patman help a lot about maintaining the change log and tags. > I'd argue that if only changelog is updated, it is _not_ a new version > of patch, and does not need changelog diff. Or maybe be less strict > policy / less strict enforcement of the policy in trivial cases. Well, if only a changelog is updated, then a [PATCH vN RESEND] should be as ok as a [PATCH vN+1], and anyway both will end up as "a new patch" for Patchwork, so the difference is not really major IMO -- meaning both should be accepted and, I believe, are accepted in practice. > Best regards, > > Pavel Amicalement,
Dear Pavel, In message <20141009230004.GA25685@amd> you wrote: > > > [1] http://www.denx.de/wiki/U-Boot/Patches > > It should really go into tree. If you think so... > It does not mention puts() vs. printf(), if it is indeed meant to be > u-boot policy. This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options. Don't we always try to use the smallest, most efficient tool that is suited for a task? Best regards, Wolfgang Denk
Dear Pavel, In message <20141009230559.GB25685@amd> you wrote: > > v2: added tags to the subject > v3: added diffs to previous version > . (From memory, but IIRC something very similar to this happened before). Yes, this happens when people repeatedly ignore to read the patch posting rules. > I'd argue that if only changelog is updated, it is _not_ a new version > of patch, and does not need changelog diff. Or maybe be less strict > policy / less strict enforcement of the policy in trivial cases. You ignore the fact that you are supposed to miimize the work load you create on reviewers. If I try to review a patch, I need to understand what has been changed compared to the previous version. Yes, of course I can apply both versions and run a diff (diffs over diffs are too difficult to read in general), but this costs time. And each reviewer has to spend this time. On the other hand, you should know exactly what you've changes, so it is minimal work for you to add such an explanation. It you use the provided tools for the task (patman comes to mind), this is even well supported. So what is better for the community: if you spend a little time once, or if every reviewer spends much more time trying to figure out what you might have changed? So it all depends whether you take the egotistic or the community point of view.... Best regards, Wolfgang Denk
Hello Wolfgang, On 10-10-14 14:22, Wolfgang Denk wrote: >> It does not mention puts() vs. printf(), if it is indeed meant to be >> u-boot policy. > This is not just U-Boot philosophy, but something that I would > consider a matter of course when writing code - using the appropriate > tools for the task at hand. If all you want to do is sendout a > constant string to the utput device, there is no need to invoke a > function that provides fancy formatting options. > > Don't we always try to use the smallest, most efficient tool that is > suited for a task? calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary. Regards, Jeroen
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote: > Hello Wolfgang, > > On 10-10-14 14:22, Wolfgang Denk wrote: > >> It does not mention puts() vs. printf(), if it is indeed meant to be > >> u-boot policy. > > > > This is not just U-Boot philosophy, but something that I would > > consider a matter of course when writing code - using the appropriate > > tools for the task at hand. If all you want to do is sendout a > > constant string to the utput device, there is no need to invoke a > > function that provides fancy formatting options. > > > > Don't we always try to use the smallest, most efficient tool that is > > suited for a task? > > calling printf("%s\n", "string") gets translated into puts by the > compiler. There should be no difference in the binary. Is this LLVM specific or does GCC do that too ? This is interesting information. Best regards, Marek Vasut
Hi Marek, On Fri, Oct 10, 2014 at 11:26 AM, Marek Vasut <marex@denx.de> wrote: >> calling printf("%s\n", "string") gets translated into puts by the >> compiler. There should be no difference in the binary. > > Is this LLVM specific or does GCC do that too ? This is interesting information. Just did a quick test here with gcc and the resulting u-boot binary size is the same when I use puts or printf.
Hello Marek, On 10-10-14 16:26, Marek Vasut wrote: > On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote: >> Hello Wolfgang, >> >> On 10-10-14 14:22, Wolfgang Denk wrote: >>>> It does not mention puts() vs. printf(), if it is indeed meant to be >>>> u-boot policy. >>> This is not just U-Boot philosophy, but something that I would >>> consider a matter of course when writing code - using the appropriate >>> tools for the task at hand. If all you want to do is sendout a >>> constant string to the utput device, there is no need to invoke a >>> function that provides fancy formatting options. >>> >>> Don't we always try to use the smallest, most efficient tool that is >>> suited for a task? >> calling printf("%s\n", "string") gets translated into puts by the >> compiler. There should be no difference in the binary > Is this LLVM specific or does GCC do that too ? This is interesting information. I was talking about gcc, it has been doing such since ages ago (unless you purposely disable it). clang does it as well. Regards, Jeroen
Hi Jeroen, On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee <jeroen@myspectrum.nl> wrote: > Hello Marek, > > On 10-10-14 16:26, Marek Vasut wrote: > > On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote: > >> Hello Wolfgang, > >> > >> On 10-10-14 14:22, Wolfgang Denk wrote: > >>>> It does not mention puts() vs. printf(), if it is indeed meant to be > >>>> u-boot policy. > >>> This is not just U-Boot philosophy, but something that I would > >>> consider a matter of course when writing code - using the appropriate > >>> tools for the task at hand. If all you want to do is sendout a > >>> constant string to the utput device, there is no need to invoke a > >>> function that provides fancy formatting options. > >>> > >>> Don't we always try to use the smallest, most efficient tool that is > >>> suited for a task? > >> calling printf("%s\n", "string") gets translated into puts by the > >> compiler. There should be no difference in the binary > > Is this LLVM specific or does GCC do that too ? This is interesting information. > > I was talking about gcc, it has been doing such since ages ago > (unless you purposely disable it). clang does it as well. That's a good thing, but generally speaking, I think that just because the compiler is being clever doesn't mean we are allowed to rely on that, because if we do take a habit of relying on the compiler being clever, two things will happen: 1) we will keep thinking the compiler is being clever even when for some reason it will stop being clever -- for instance, because someone decided to disable the clever feature; 2) we will begin thinking the compiler is clever in situations where it never has and never will. IMO, a quick cost/benefit comparison of choosing between manually turning printf() into puts whenever doable vs letting the compiler do the changes automatically, the manual option wins -- it's bit like Pascal's Wager: you don't lose much but you can only win. > Regards, > Jeroen > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Amicalement,
Hello Albert, On 10-10-14 21:51, Albert ARIBAUD wrote: > Hi Jeroen, > > On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee > <jeroen@myspectrum.nl> wrote: > >> Hello Marek, >> >> On 10-10-14 16:26, Marek Vasut wrote: >>> On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote: >>>> Hello Wolfgang, >>>> >>>> On 10-10-14 14:22, Wolfgang Denk wrote: >>>>>> It does not mention puts() vs. printf(), if it is indeed meant to be >>>>>> u-boot policy. >>>>> This is not just U-Boot philosophy, but something that I would >>>>> consider a matter of course when writing code - using the appropriate >>>>> tools for the task at hand. If all you want to do is sendout a >>>>> constant string to the utput device, there is no need to invoke a >>>>> function that provides fancy formatting options. >>>>> >>>>> Don't we always try to use the smallest, most efficient tool that is >>>>> suited for a task? >>>> calling printf("%s\n", "string") gets translated into puts by the >>>> compiler. There should be no difference in the binary >>> Is this LLVM specific or does GCC do that too ? This is interesting information. >> I was talking about gcc, it has been doing such since ages ago >> (unless you purposely disable it). clang does it as well. > That's a good thing, but generally speaking, I think that just because > the compiler is being clever doesn't mean we are allowed to rely on > that, because if we do take a habit of relying on the compiler being > clever, two things will happen: Why can't this be relied on, I gave up digging if this is a gcc 3 or 2 feature. It is old at least, museum stuff if it is not supported. > 1) we will keep thinking the compiler is being clever even when for > some reason it will stop being clever -- for instance, because someone > decided to disable the clever feature; If you ask to disable it, it is good if it does so, don't see a problem with that. Anyway, it is not an u-boot issue, anything below -O2 is not supported anyway. > 2) we will begin thinking the compiler is clever in situations where it > never has and never will. I would almost take this as an insult, I hope u-boot folks know or at least check before they assume a compiler does XYZ. And yes compilers will replace simple printf call with their simpler equivalent and has been doing so for quite a while (and that is an understatement). > IMO, a quick cost/benefit comparison of choosing between manually > turning printf() into puts whenever doable vs letting the compiler do > the changes automatically, the manual option wins -- it's bit like > Pascal's Wager: you don't lose much but you can only win. No it is the other way around; why on earth do you want demand patch submitters to make changes which result in the exactly same binary; you waste time of reviewers / patch submitter and it doesn't serve a goal. So to turn it around: just use printf: "you don't lose much but you can only win." Regards, Jeroen
Hi Jeroen, On Fri, 10 Oct 2014 22:40:48 +0200, Jeroen Hofstee <dasuboot@myspectrum.nl> wrote: > Hello Albert, > > On 10-10-14 21:51, Albert ARIBAUD wrote: > > Hi Jeroen, > > > > On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee > > <jeroen@myspectrum.nl> wrote: > > > >> Hello Marek, > >> > >> On 10-10-14 16:26, Marek Vasut wrote: > >>> On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote: > >>>> Hello Wolfgang, > >>>> > >>>> On 10-10-14 14:22, Wolfgang Denk wrote: > >>>>>> It does not mention puts() vs. printf(), if it is indeed meant to be > >>>>>> u-boot policy. > >>>>> This is not just U-Boot philosophy, but something that I would > >>>>> consider a matter of course when writing code - using the appropriate > >>>>> tools for the task at hand. If all you want to do is sendout a > >>>>> constant string to the utput device, there is no need to invoke a > >>>>> function that provides fancy formatting options. > >>>>> > >>>>> Don't we always try to use the smallest, most efficient tool that is > >>>>> suited for a task? > >>>> calling printf("%s\n", "string") gets translated into puts by the > >>>> compiler. There should be no difference in the binary > >>> Is this LLVM specific or does GCC do that too ? This is interesting information. > >> I was talking about gcc, it has been doing such since ages ago > >> (unless you purposely disable it). clang does it as well. > > That's a good thing, but generally speaking, I think that just because > > the compiler is being clever doesn't mean we are allowed to rely on > > that, because if we do take a habit of relying on the compiler being > > clever, two things will happen: > > Why can't this be relied on, I gave up digging if this is a gcc 3 or 2 > feature. It is old at least, museum stuff if it is not supported. It's a general habit I have of never assuming anything is forever. > > 1) we will keep thinking the compiler is being clever even when for > > some reason it will stop being clever -- for instance, because someone > > decided to disable the clever feature; > > If you ask to disable it, it is good if it does so, don't see a problem > with that. Anyway, it is not an u-boot issue, anything below -O2 is not > supported anyway. There is no problem if the person disabling the feature is also the one doing assumptions on the feature state. Problems arise when these are two different persons, because the one relying on the feature might not know or notice that the other one disabled it. > > 2) we will begin thinking the compiler is clever in situations where it > > never has and never will. > > I would almost take this as an insult, I hope u-boot folks know or at > least check before they assume a compiler does XYZ. And yes > compilers will replace simple printf call with their simpler equivalent > and has been doing so for quite a while (and that is an understatement). I certainly did not intend this as an insult. But no, if I personally had encountered a printf("%s\n", s) in the course of some development, I would not have gone and looked whether the compiler might turn it into a puts(s). I'd have assumed it does not, because assuming so was the fastest and safest approach. Don't misunderstand me: I'm quite pleased that gcc and LLVM do that. And I'm also quite convinced that it's a good thing -- actually, I did start by saying so. The problem I see is not specifically with the printf()/puts() case, it is with the *general* assumption that a compiler is clever -- and again, I did start my opinion with "generally speaking". The problem I see is that a developer will not necessarily go and test every single thing that the compiler *might* be clever enough to do (or dump enough not to); a developer will assume some of it, and whenever he assumes, he should assume a dumb compiler. > > IMO, a quick cost/benefit comparison of choosing between manually > > turning printf() into puts whenever doable vs letting the compiler do > > the changes automatically, the manual option wins -- it's bit like > > Pascal's Wager: you don't lose much but you can only win. > > No it is the other way around; why on earth do you want demand > patch submitters to make changes which result in the exactly same > binary; you waste time of reviewers / patch submitter and it doesn't > serve a goal. > > So to turn it around: just use printf: "you don't lose much but you > can only win." In the specific case of printf()/puts(), a wager is meaningless since the compiler capability is already known. Therefore, there is no reason to "wager on 'just us[ing] printf'"; there is reason to "just use printf because it is known to turn into puts() if possible". But please remember that I considered the wager *only* in a *general* case where a developer could replace some code with some other, more efficient code, and does *not* know whether the compiler could be clever enough to do the replacement by itself. The developer /might/ check the compiler... or he might not check, and instead, wager on the compiler being clever or dumb; and in this *general* case, he should wager on a dumb compiler. Of course, if the case is specific and known to him, i.e. he already knows whether the compiler will be clever or dumb in that specific case, then there is *no* wager to be done; the developer should just act on his knowledge (and check it again from time to time, but that's somewhat beside the point). > Regards, > Jeroen Amicalement,
Dear Jeroen, In message <5437E778.3050306@myspectrum.nl> you wrote: > > calling printf("%s\n", "string") gets translated into puts by the > compiler. There should be no difference in the binary. Interesting, I didn't know that. Is this somewhere documented? Is there any comprehensive list of such "magic" optimizations? Best regards, Wolfgang Denk
Dear Jeroen, In message <54384450.3000204@myspectrum.nl> you wrote: > > If you ask to disable it, it is good if it does so, don't see a problem > with that. Anyway, it is not an u-boot issue, anything below -O2 is not > supported anyway. I'm not sure what you mean here. Gcc certainly does this replacement with -Os as used for U-Boot. > I would almost take this as an insult, I hope u-boot folks know or at > least check before they assume a compiler does XYZ. And yes > compilers will replace simple printf call with their simpler equivalent > and has been doing so for quite a while (and that is an understatement). I wonder how many people know about this - and where it is documented? > So to turn it around: just use printf: "you don't lose much but you > can only win." Sorry, but I disagree here. First, we have a compatibility problem here. GCC assumes that puts() will add a newline character after the string; U-Boot puts() does NOT do this. So the GCC auto-converted printf()s will all be wrong, as they are missing the newline. [1] Second, using puts() insteas of printf() is also a means of documenting your code. It shows your intention to print a constant string. Just one example: compare A: void add_header(const char *s) { printf("MY HEADER: %s\n", s); } versus B: void add_header(const char *s) { puts("MY HEADER: "); puts(s); /* Assuming U-Boot puts() - no automatic \n added */ putc('\n'); } Which is "better"? A is obviously much shorter and more elegant; but B is much more robust - A will happily crash your system when you try to print a string like "s%s%s%s%s%s%s%s%s%s%s" (not to mention that this may open a classic attack vector to break into a running system). So yes, it does make sense to explicitly use puts(). [1] One might argue that this is a bug in U-Boot and should be fixed, but that is another topic. Best regards, Wolfgang Denk
Dear Jeroen, In message <20141011150346.150C038352A@gemini.denx.de> i wrote: > > Which is "better"? A is obviously much shorter and more elegant; but > B is much more robust - A will happily crash your system when you try > to print a string like "s%s%s%s%s%s%s%s%s%s%s" (not to mention that > this may open a classic attack vector to break into a running system). Ignore me. This example was obviously crap. What I had in mind was something where you would use char *s; ... printf(s); Sorry... Best regards, Wolfgang Denk
Hello Wolfgang / Albert / others, On 10-10-14 16:04, Jeroen Hofstee wrote: > Hello Wolfgang, > > On 10-10-14 14:22, Wolfgang Denk wrote: >>> It does not mention puts() vs. printf(), if it is indeed meant to be >>> u-boot policy. >> This is not just U-Boot philosophy, but something that I would >> consider a matter of course when writing code - using the appropriate >> tools for the task at hand. If all you want to do is sendout a >> constant string to the utput device, there is no need to invoke a >> function that provides fancy formatting options. >> >> Don't we always try to use the smallest, most efficient tool that is >> suited for a task? > > calling printf("%s\n", "string") gets translated into puts by the > compiler. There should be no difference in the binary. mumbles: while this is true in general it won't hold for u-boot since -ffreestanding disables such rewrites and u-boot is compiled with that flag. On the bright side, perhaps I educated some people a bit that they are wasting time rewriting such lines in normal, hosted applications. Regards, Jeroen
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 @@ -0,0 +1,11 @@ +Differences from kernel: + +* SPDX license headers are required. + +* puts() is preffered over single-argument prinf() + +* later versions of patch should come with "diff changelog" below "---" + +* subject should begin with tags, such as "arm: socfpga:" + +* should pass checkpatch