bugGNU roff - Bugs: bug #60639, ...

 
 

bug #60639: src/utils/addftinfo/addftinfo.cpp:119: double increment ?

Submitter:  David Binderman <dcb314>
Submitted:  Wed 19 May 2021 06:45:58 PM UTC
   
 
Category:  Utilities Severity:  1 - Wish
Item Group:  Warning/Suspicious behaviour Status:  Invalid
Privacy:  Public Assigned to:  schwarze
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 10 Jun 2022 10:55:44 PM UTC, comment #6: 


comment #5:

> Our forthcoming release improves the usage message (IMO) but not this validation step.
>


> $ ./build/addftinfo -asc-height -body-depth 60
> usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N] [-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N] RESOLUTION UNIT-WIDTH FONT
> usage: ./build/addftinfo -v
> usage: ./build/addftinfo --version
> $ ./build/addftinfo -body-depth 60 -asc-depth
> usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N] [-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N] RESOLUTION UNIT-WIDTH FONT
> usage: ./build/addftinfo -v
> usage: ./build/addftinfo --version


>
> Maybe I'll fix this for grins.


Filed as bug #60661.

G. Branden Robinson <gbranden>
Group administrator
Fri 21 May 2021 01:11:44 AM UTC, comment #5: 

Thanks for the quick analysis, Ingo.

comment #1:

> The for loops were revised (by original author James Clark) in 1995--not to add redundant incrementations, but rather to dumb the code down to ISO C90 from what was then a GCC extension.


Ironically, as part of my efforts to improve diagnostic messages from the groff system, I added a loop-scoped index variable as part of commit 2a4c8d7475f61f9056c0a25b5ae68843e3c7fbe7 (29 April 2020), just like the kind James Clark got rid of in 1995.

comment #4:

> > I think i += 2 in the last part of the for loop specifier
> > might make the programmer's original intent more clear.
>
> Purely a matter of style, there are arguments both ways.
> The existing code puts each increment close to the code
> handling the respective argument, and your suggestion
> would increase the risk of causing bugs when introducing
> the first option that does not take an argument.


I think this point is relatively weak given addftinfo's age and purpose.  I don't think anyone expects exciting developments in the AT&T device-independent troff font format that our utility will need to adapt to.  But usually, yes, it's a much stronger consideration.

> Changing code just because a compiler has suspicions is not a
> good idea - unless the code is indeed unambiguously bad style
> or buggy and the compiler merely finds a real problem.  But the
> only question that ever matters is "is there a real problem
> with the code" and never "does the compiler complain".  The
> compiler is only a tool and must never be used blindly.


This, I agree with as well.

It's a little disappointing that Clang wasn't bright enough to analyze the code paths through the for loop and determine that at least one of them does NOT result in the double increment.


   115      for (j = 0;; j++) {
   116        if (j >= sizeof(param_table)/sizeof(param_table[0]))
   117          fatal("parameter '%1' not recognized", argv[i] + 1);
   118        if (strcmp(param_table[j].name, argv[i] + 1) == 0)
   119          break;
   120      }


Also, I don't think lines 112-113 work as intended.


$ addftinfo -asc-height -body-depth 60
usage: addftinfo [-v] [-param value] ... resolution unitwidth font
$ addftinfo -body-depth 60 -asc-depth
usage: addftinfo [-v] [-param value] ... resolution unitwidth font


Our forthcoming release improves the usage message (IMO) but not this validation step.


$ ./build/addftinfo -asc-height -body-depth 60
usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N] [-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N] RESOLUTION UNIT-WIDTH FONT
usage: ./build/addftinfo -v
usage: ./build/addftinfo --version
$ ./build/addftinfo -body-depth 60 -asc-depth
usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N] [-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N] RESOLUTION UNIT-WIDTH FONT
usage: ./build/addftinfo -v
usage: ./build/addftinfo --version


Maybe I'll fix this for grins.

All that said, I'm open to considering a specialized comment or pragma push that shuts up this compiler warning.  If David's seen it--more people will.

G. Branden Robinson <gbranden>
Group administrator
Thu 20 May 2021 10:08:01 PM UTC, comment #4: 

comment #3:

> I think i += 2 in the last part of the for loop specifier
> might make the programmer's original intent more clear.


Purely a matter of style, there are arguments both ways.
The existing code puts each increment close to the code
handling the respective argument, and your suggestion
would increase the risk of causing bugs when introducing
the first option that does not take an argument.

Changing code just because a compiler has suspicions is not a
good idea - unless the code is indeed unambiguously bad style
or buggy and the compiler merely finds a real problem.  But the
only question that ever matters is "is there a real problem
with the code" and never "does the compiler complain".  The
compiler is only a tool and must never be used blindly.

Ingo Schwarze <schwarze>
Group Member
Thu 20 May 2021 05:42:32 PM UTC, comment #3: 

comment #2:

> Well, it is trivial to see that incrementing twice is correct
> given the purpose of the code: once for the option and once for > the argument.


Thanks for the clarification. I had my suspicions.

> David, it is well known that modern C compilers emit huge
> amounts of badly misleading, totally bogus warnings by default.


Thanks for your opinion. I suspect that developers of modern
C compilers might have something to say about that.

> Please never report compiler warnings to any project unless you >have checked that the code is really wrong,


I think i += 2 in the last part of the for loop specifier
might make the programmer's original intent more clear.

Something like:

  for (i = 1; i < argc && argv[i][0] == '-'; i += 2) {

I can't imagine this change makes much difference at run time, however.

David Binderman <dcb314>
Thu 20 May 2021 05:24:22 PM UTC, comment #2: 

Well, it is trivial to see that incrementing twice is correct given the purpose of the code: once for the option and once for the argument.

> I just tried to compile the package with clang. It said:
> src/utils/addftinfo/addftinfo.cpp:119:5: warning:
> variable 'i' is incremented both in the loop header
> and in the loop body [-Wfor-loop-analysis]


David, it is well known that modern C compilers emit huge amounts of badly misleading, totally bogus warnings by default.

Please never report compiler warnings to any project unless you have checked that the code is really wrong, and always describe in detail why you think the code is wrong if you think it is, in terms different from whatever the compiler says.

Ingo Schwarze <schwarze>
Group Member
Thu 20 May 2021 07:33:45 AM UTC, comment #1: 

Thanks for the report, David!

Assigning to the new "Utilities" category I just created.

I haven't analyzed the correctness of this code yet, but here's some more context.  This is a hand-rolled option parser.


   107    for (i = 1; i < argc && argv[i][0] == '-'; i++) {
   108      if (argv[i][1] == '-' && argv[i][2] == '\0') {
   109        i++;
   110        break;
   111      }
   112      if (i + 1 >= argc)
   113        usage("option requires argument");
   114      size_t j;
   115      for (j = 0;; j++) {
   116        if (j >= sizeof(param_table)/sizeof(param_table[0]))
   117          fatal("parameter '%1' not recognized", argv[i] + 1);
   118        if (strcmp(param_table[j].name, argv[i] + 1) == 0)
   119          break;
   120      }
   121      if (sscanf(argv[i+1], "%d", &(param.*(param_table[j].par))) != 1)
   122        fatal("invalid option argument '%1'", argv[i+1]);
   123      i++;
   124    }
   125    if (argc - i != 3)
   126      usage("insufficient arguments");


This code is very old.  Almost none of it has changed since the dawn of our repository's history on 2 June 1991.

The for loops were revised (by original author James Clark) in 1995--not to add redundant incrementations, but rather to dumb the code down to ISO C90 from what was then a GCC extension.


@@ -89,14 +89,16 @@ int main(int argc, char **argv)
   param.comma_depth = DEFAULT_COMMA_DEPTH;
   param.desc_depth = DEFAULT_DESC_DEPTH;
   param.body_depth = DEFAULT_BODY_DEPTH;
-  for (int i = 1; i < argc && argv[i][0] == '-'; i++) {
+  int i;
+  for (i = 1; i < argc && argv[i][0] == '-'; i++) {
     if (argv[i][1] == '-' && argv[i][2] == '\0') {
       i++;
       break;
     }
     if (i + 1 >= argc)
       usage();
-    for (int j = 0;; j++) {
+    int j;
+    for (j = 0;; j++) {
       if (j >= sizeof(param_table)/sizeof(param_table[0]))
        fatal("parameter `%1' not recognized", argv[i] + 1);
       if (strcmp(param_table[j].name, argv[i] + 1) == 0)



G. Branden Robinson <gbranden>
Group administrator
Wed 19 May 2021 06:45:58 PM UTC, original submission:  

I just tried to compile the package with clang. It said:

src/utils/addftinfo/addftinfo.cpp:119:5: warning: variable 'i' is incremented both in the loop header and in the loop body [-Wfor-loop-analysis]

Source code is

  for (i = 1; i < argc && argv[i][0] == '-'; i++) {
...
    i++;
  }

David Binderman <dcb314>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by dcb314 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-06-10 gbranden Assigned toNone schwarze
    2021-05-20 schwarze Severity3 - Normal 1 - Wish
        Item GroupNone Warning/Suspicious behaviour
        StatusNeed Info Invalid
        Open/ClosedOpen Closed
    2021-05-20 gbranden CategoryNone Utilities
        StatusNone Need Info

    Back to the top

    Powered by Savane 3.15-e6e5.
    Corresponding source code