Skip to content

MAINT: f2py: don't generate code that triggers -Wsometimes-uninitialized#20940

Merged
rgommers merged 3 commits intonumpy:mainfrom
rgommers:f2py-warnings
Feb 1, 2022
Merged

MAINT: f2py: don't generate code that triggers -Wsometimes-uninitialized#20940
rgommers merged 3 commits intonumpy:mainfrom
rgommers:f2py-warnings

Conversation

@rgommers
Copy link
Copy Markdown
Member

Warnings all look like:

scipy/linalg/_flapackmodule.c:2200:9: warning: variable 'return_value' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (capi_j>capi_i)
        ^~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2217:12: note: uninitialized use occurs here
    return return_value;
           ^~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2200:5: note: remove the 'if' if its condition is always true
    if (capi_j>capi_i)
    ^~~~~~~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2099:21: note: initialize the variable 'return_value' to silence this warning
    int return_value;
                    ^
                     = 0

…ized`

Warnings look like:

```
scipy/linalg/_flapackmodule.c:2200:9: warning: variable 'return_value' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (capi_j>capi_i)
        ^~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2217:12: note: uninitialized use occurs here
    return return_value;
           ^~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2200:5: note: remove the 'if' if its condition is always true
    if (capi_j>capi_i)
    ^~~~~~~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2099:21: note: initialize the variable 'return_value' to silence this warning
    int return_value;
                    ^
                     = 0
```
@charris
Copy link
Copy Markdown
Member

charris commented Jan 29, 2022

I take it you would like this backported?

@charris
Copy link
Copy Markdown
Member

charris commented Jan 29, 2022

Is that the correct return value in those cases?

@rgommers
Copy link
Copy Markdown
Member Author

Is that the correct return value in those cases?

Not sure, it may be that something else is missing. That code is pretty much unreadable.

@rgommers
Copy link
Copy Markdown
Member Author

Figure = 0 is better than uninitialized use

@charris
Copy link
Copy Markdown
Member

charris commented Jan 29, 2022

Not sure, it may be that something else is missing.

I'll leave it to 1.22.3 then. I want to get the initial 1.22.x problems fixed up as soon as possible so we can deal with the more obscure ones.

@charris
Copy link
Copy Markdown
Member

charris commented Jan 30, 2022

@HaoZeke @pearu Thoughts?

@HaoZeke
Copy link
Copy Markdown
Member

HaoZeke commented Jan 30, 2022

Since this code is generated it would be better to initialize it within the if condition instead of unconditionally. The warning suggests that F2PY codegen can be improved and initializing it to zero would prematurely silence warnings when better fixes might make sense.

Basically I would prefer to fix the return statement generation than the initialization.

@pearu
Copy link
Copy Markdown
Contributor

pearu commented Jan 31, 2022

Background: the referenced code corresponds to call-back functions that a Fortran function uses to call Python functions. Normally, a non-void call-back function is expected to return a value, that is, the user-provided Python function ought to return a numeric value. However, if it returns None (as specified by a user), then (capi_j > capi_i) is false and the value of return_value is undefined. Since this undefined behavior is under the user's control, f2py allows it, although it will be silent. Initializing return_value to 0 (for the sake of silencing the compiler) makes this undefined behavior even more silent.

To fix this issue, we have the following objectives:

  1. As a primary one, enforce users to write well-defined code (non-void call-backs should always return a value)
  2. Silence the compiler as a secondary objective (initialize return_value)
  3. The change should be non-BC-breaking (cannot raise an exception when a user callback does not provide a return value, for instance)

I suggest the following solution:

  1. When a non-void user callback does not provide a return value, trigger a warning. The corresponding code should be inserted here as else block to if (capi_j > capi_i):
    ' if (capi_j>capi_i)\n GETSCALARFROMPYTUPLE(capi_return,capi_i++,&return_value,#ctype#,"#ctype#_from_pyobj failed in converting return_value of call-back function #name# to C #ctype#\\n");',
  2. Initialize return_value. This will be trickier as the return type can be any scalar, including complex. The initialization can be carried out in the same else block as in 1.

Copy link
Copy Markdown
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle complex return value case and trigger a warning if user-provided cb function does return a value.

@rgommers
Copy link
Copy Markdown
Member Author

Thanks @pearu. Do you just want to push to this PR? I'm happy for you to take it over. My change is just a minimal "make annoying warnings go away", but I have little desire to make further changes.

@pearu
Copy link
Copy Markdown
Contributor

pearu commented Jan 31, 2022

@rgommers how about this: I'll push changes to your branch and make sure that numpy CI is green, and you'll take care of testing the scipy part?

@rgommers
Copy link
Copy Markdown
Member Author

Sure, that sounds good!

@pearu pearu self-requested a review January 31, 2022 13:51
Copy link
Copy Markdown
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgommers please review.

Copy link
Copy Markdown
Member Author

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, SciPy build is much happier and the generated code is now readable which is useful too. Thanks @pearu!

@rgommers rgommers merged commit bcdfd20 into numpy:main Feb 1, 2022
@rgommers rgommers deleted the f2py-warnings branch February 1, 2022 17:26
@rgommers rgommers added the 09 - Backport-Candidate PRs tagged should be backported label Feb 1, 2022
@rgommers rgommers added this to the 1.23.0 release milestone Feb 1, 2022
charris pushed a commit to charris/numpy that referenced this pull request Feb 2, 2022
…ized` (numpy#20940)

* MAINT: f2py: don't  generate code that triggers `-Wsometimes-uninitialized`

Warnings look like:

```
scipy/linalg/_flapackmodule.c:2200:9: warning: variable 'return_value' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (capi_j>capi_i)
        ^~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2217:12: note: uninitialized use occurs here
    return return_value;
           ^~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2200:5: note: remove the 'if' if its condition is always true
    if (capi_j>capi_i)
    ^~~~~~~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2099:21: note: initialize the variable 'return_value' to silence this warning
    int return_value;
                    ^
                     = 0
```

Also:

- Initialize complex return value.
- Warn on non-void callback returning None.
- Use brackets in if-else block.

This makes the code more readable.

Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
@charris charris changed the title MAINT: f2py: don't generate code that triggers -Wsometimes-uninitialized MAINT: f2py: don't generate code that triggers -Wsometimes-uninitialized Feb 2, 2022
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 2, 2022
@charris charris removed this from the 1.23.0 release milestone Feb 2, 2022
melissawm pushed a commit to melissawm/numpy that referenced this pull request Apr 12, 2022
…ized` (numpy#20940)

* MAINT: f2py: don't  generate code that triggers `-Wsometimes-uninitialized`

Warnings look like:

```
scipy/linalg/_flapackmodule.c:2200:9: warning: variable 'return_value' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (capi_j>capi_i)
        ^~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2217:12: note: uninitialized use occurs here
    return return_value;
           ^~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2200:5: note: remove the 'if' if its condition is always true
    if (capi_j>capi_i)
    ^~~~~~~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2099:21: note: initialize the variable 'return_value' to silence this warning
    int return_value;
                    ^
                     = 0
```

Also:

- Initialize complex return value.
- Warn on non-void callback returning None.
- Use brackets in if-else block.

This makes the code more readable.

Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
seberg pushed a commit to seberg/numpy that referenced this pull request Apr 24, 2022
…ized` (numpy#20940)

* MAINT: f2py: don't  generate code that triggers `-Wsometimes-uninitialized`

Warnings look like:

```
scipy/linalg/_flapackmodule.c:2200:9: warning: variable 'return_value' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (capi_j>capi_i)
        ^~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2217:12: note: uninitialized use occurs here
    return return_value;
           ^~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2200:5: note: remove the 'if' if its condition is always true
    if (capi_j>capi_i)
    ^~~~~~~~~~~~~~~~~~
scipy/linalg/_flapackmodule.c:2099:21: note: initialize the variable 'return_value' to silence this warning
    int return_value;
                    ^
                     = 0
```

Also:

- Initialize complex return value.
- Warn on non-void callback returning None.
- Use brackets in if-else block.

This makes the code more readable.

Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants