[cling] Ignore -Wunused-result in wrapped code #12654

jalopezg-git · 2023-04-13T10:42:22Z

Make FilteringDiagConsumer also ignore -Wunused-result. Whether or not such diagnostic is filtered depends on CompilationOptions::IgnorePromptDiags.

In particular, IgnorePromptDiags should only be enabled for code parsed via Interpreter::EvaluateInternal(). Thus, as of this commit IgnorePromptDiags defaults to 0 in makeDefaultCompilationOpts()

The observable effect of this change is ignoring -Wunused-result for wrapped code, e.g.

[[nodiscard]] int f() { return 0; }

// This yields `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]`
void g() { f(); }

f(); // but this should not

Alternatively, as discussed with @Axel-Naumann, we could insert #pragma clang diagnositc ... directives in Interpreter::WrapInput(), but I see that as much less elegant.

Checklist:

tested changes locally
the patch passes cling tests

This PR fixes #11562.

Make `FilteringDiagConsumer` also ignore -Wunused-result. Whether or not such diagnostic is filtered depends on `CompilationOptions::IgnorePromptDiags`. In particular, `IgnorePromptDiags` should _only_ be enabled for code parsed via `Interpreter::EvaluateInternal()`. Thus, as of this commit `IgnorePromptDiags` defaults to 0 in `makeDefaultCompilationOpts()` The observable effect of this change is ignoring `-Wunused-result` for wrapped code, e.g. ```c++ [[nodiscard]] int f() { return 0; } // This yields `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]` void g() { f(); } f(); // but this should not ```

phsft-bot · 2023-04-13T10:42:30Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2023-04-13T11:12:28Z

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.core.roottest_root_core_execStatusBitsCheck

phsft-bot · 2023-04-13T11:19:45Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-04-13T11:21:33Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-04-13T11:23:11Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-04-13T11:38:41Z

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Failing tests:

jalopezg-git · 2023-04-13T11:45:35Z

Both tests fail due to the change in the default value for IgnorePromptDiags, but see comments below.

projectroot.roottest.root.dataframe.roottest_root_dataframe_test_ROOT9975

Taking a look at the test, to return something from the entry point of a macro seems legal even if the function is void-returning. I guess that's why https://github.com/root-project/root/blob/master/interpreter/cling/lib/Interpreter/IncrementalParser.cpp#L173 is there.
I would vote for either dropping that legacy behavior or always filtering the diagnostic. Any personal preferences, @Axel-Naumann @hahnjo @vgvassilev ?

projectroot.roottest.root.core.roottest_root_core_execStatusBitsCheck

This seems a legitimate error that was being filtered (see here); I think the test should be fixed.

phsft-bot · 2023-04-13T12:11:09Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.dataframe.roottest_root_dataframe_test_ROOT9975

phsft-bot · 2023-04-13T12:12:05Z

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

github-actions · 2023-04-13T12:28:52Z

Test Results

        5 files         5 suites 1d 3h 29m 2s ⏱️
  2 420 tests   2 413 ✔️ 0 💤   7 ❌
11 942 runs 11 912 ✔️ 0 💤 30 ❌

For more details on these failures, see this check.

Results for commit 4045776.

pcanal · 2023-04-15T00:43:51Z

If I understand correctly, with this patch neither:

// This yields `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]`
void g() { f(); }

f(); // but this should not

will issue the warning ... which seems wrong.

The issue being addressed is:

root [1] v.size()
ROOT_prompt_1:1:1: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
v.size()
^~~~~~
(unsigned long) 0

where we are in a use case where the warning is literally speaking incorrect since we do use the value and actually print it.

i.e. Semantically, it seems that:

root [1] v.size();

should/could issue the warning while

root [1] v.size()

should definitively not issue the warning.

So a genuine question is "is the fix here 'too broad' ? " and/or "is the "better" fix so expensive that it is better overall to suppress the warning globally?"

pcanal · 2023-04-15T00:46:38Z

Both tests fail due to the change in the default value for IgnorePromptDiags

I agree that both tests should be fixed to not have functions that returns a value when declared to return void.
However

{
   return 0;
}

should continue to work (i.e. return in unnamed macros)

jalopezg-git · 2023-04-17T18:57:45Z

If I understand correctly, with this patch neither:

// This yields `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]`
void g() { f(); }

f(); // but this should not

Actually, the first line does issue a warning (which is expected), while it is suppressed in the second case.

The issue being addressed is:

root [1] v.size()
ROOT_prompt_1:1:1: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
v.size()
^~~~~~
(unsigned long) 0

I have pasted below the current state of affairs after applying the patch in this PR (i.e., -Wunused-result, and others should be filtered only for wrapped code).

root [0] std::vector<int> v;
root [1] v.size()
(unsigned long) 0
root [2] // `return` was intentionally ommitted in f() below
root [3] size_t f() { v.size(); }
ROOT_prompt_3:1:14: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
size_t f() { v.size(); }
             ^~~~~~
ROOT_prompt_3:1:24: warning: non-void function does not return a value [-Wreturn-type]
size_t f() { v.size(); }
                       ^

So a genuine question is "is the fix here 'too broad' ? " and/or "is the "better" fix so expensive that it is better overall to suppress the warning globally?"

It's a bit too broad, as it suppresses the warning in wrapped code even if the expression is not part of value capture/printing.

Given that the diagnostic is emitted in SemaExpr when the expression is parsed, the ideal solution would be to buffer diagnostics and then selectively drop this one depending on the source location. But that is probably not worth the additional complexity. 🙂

pcanal · 2023-04-17T19:06:06Z

ok fair enough. This looks good then. Did you also test going through the TMethodCall interfaces?

jalopezg-git · 2023-04-17T19:09:33Z

However
{
   return 0;
}
should continue to work (i.e. return in unnamed macros)

And I confirm that it does, as code in unnamed macros is wrapped as needed.

jalopezg-git · 2023-04-17T19:19:14Z

ok fair enough. This looks good then. Did you also test going through the TMethodCall interfaces?

I think TMethodCall uses the TClingCallFunc interface, and if I am not mistaken, it was addressed by this PR: #9244.

vgvassilev · 2023-04-17T19:23:13Z

@wlav, fyi.

vgvassilev · 2023-04-17T19:24:57Z

Does having nodiscard avoid stack corruption if we still forget to return a result?

jalopezg-git · 2023-04-17T19:28:49Z

Does having nodiscard avoid stack corruption if we still forget to return a result?

No, I don't think it has any effect on that.

jalopezg-git · 2023-04-18T11:08:44Z

And I confirm that it does, as code in unnamed macros is wrapped as needed.

Unless @Axel-Naumann, @vgvassilev or @hahnjo suggest otherwise, the only thing missing is to fix the two failing tests reported above.

EDIT: sibling roottest PR is root-project/roottest#951.

As of PR root-project/root#12654, returning a non-`void` expression from `void`-returning function is only valid in an unnamed macro (in which case the diagnostic is filtered).

jalopezg-git · 2023-04-19T16:25:15Z

@phsft-bot build

phsft-bot · 2023-04-19T16:25:21Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

As of PR root-project/root#12654, returning a non-`void` expression from `void`-returning function is only valid in an unnamed macro (in which case the diagnostic is filtered).

hahnjo

LG

interpreter/cling/lib/Interpreter/Interpreter.cpp

As of PR root-project/root#12654, returning a non-`void` expression from `void`-returning function is only valid in an unnamed macro (in which case the diagnostic is filtered). Cherry-picked from a905ea5.

This diagnostic should always be promoted to error, regardless of the ignoring state in `FilteringDiagConsumer`. This fixes the SourceCall/ErrorMacro.C test. The failure became visible after merging root-project#12654, given that `IgnorePromptDiags` now defaults to 0 in `makeDefaultCompilationOptions()`.

This diagnostic should always be promoted to error, regardless of the ignoring state in `FilteringDiagConsumer`. This fixes the SourceCall/ErrorMacro.C test. The failure became visible after merging #12654, given that `IgnorePromptDiags` now defaults to 0 in `makeDefaultCompilationOptions()`.

This diagnostic should always be promoted to error, regardless of the ignoring state in `FilteringDiagConsumer`. This fixes the SourceCall/ErrorMacro.C test. The failure became visible after merging root-project/root#12654, given that `IgnorePromptDiags` now defaults to 0 in `makeDefaultCompilationOptions()`.

This diagnostic should always be promoted to error, regardless of the ignoring state in `FilteringDiagConsumer`. This fixes the SourceCall/ErrorMacro.C test. The failure became visible after merging root-project#12654, given that `IgnorePromptDiags` now defaults to 0 in `makeDefaultCompilationOptions()`.

jalopezg-git added improvement in:Cling labels Apr 13, 2023

jalopezg-git requested review from Axel-Naumann, hahnjo and vgvassilev April 13, 2023 10:42

jalopezg-git self-assigned this Apr 13, 2023

jalopezg-git requested a review from pcanal April 13, 2023 12:59

jalopezg-git mentioned this pull request Apr 19, 2023

[core][DF] Fix -Wreturn-type in tests root-project/roottest#951

Merged

hahnjo approved these changes Apr 20, 2023

View reviewed changes

interpreter/cling/lib/Interpreter/Interpreter.cpp Show resolved Hide resolved

jalopezg-git merged commit ade784d into root-project:master Apr 20, 2023
11 of 14 checks passed

jalopezg-git deleted the cling-diag-Wunused-result branch April 20, 2023 13:08

This was referenced Apr 24, 2023

[core][DF] Fix -Wreturn-type in tests root-project/roottest#952

Merged

[cling][v6-28] Ignore -Wunused-result in wrapped code #12703

Merged

This was referenced May 19, 2023

[cling] Fix a number of failures in cling tests #12855

Merged

[cling][v6-28] Fix a number of failures in cling tests #12856

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cling] Ignore -Wunused-result in wrapped code #12654

[cling] Ignore -Wunused-result in wrapped code #12654

jalopezg-git commented Apr 13, 2023 •

edited

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

jalopezg-git commented Apr 13, 2023 •

edited

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

github-actions bot commented Apr 13, 2023

pcanal commented Apr 15, 2023 •

edited

pcanal commented Apr 15, 2023 •

edited

jalopezg-git commented Apr 17, 2023

pcanal commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023 •

edited

vgvassilev commented Apr 17, 2023

vgvassilev commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023

jalopezg-git commented Apr 18, 2023 •

edited

jalopezg-git commented Apr 19, 2023

phsft-bot commented Apr 19, 2023

hahnjo left a comment

[cling] Ignore -Wunused-result in wrapped code #12654

[cling] Ignore -Wunused-result in wrapped code #12654

Conversation

jalopezg-git commented Apr 13, 2023 • edited

Checklist:

phsft-bot commented Apr 13, 2023

phsft-bot commented Apr 13, 2023

Failing tests:

phsft-bot commented Apr 13, 2023

Failing tests:

phsft-bot commented Apr 13, 2023

Failing tests:

phsft-bot commented Apr 13, 2023

Failing tests:

phsft-bot commented Apr 13, 2023

Failing tests:

jalopezg-git commented Apr 13, 2023 • edited

phsft-bot commented Apr 13, 2023

Failing tests:

phsft-bot commented Apr 13, 2023

Failing tests:

github-actions bot commented Apr 13, 2023

Test Results

pcanal commented Apr 15, 2023 • edited

pcanal commented Apr 15, 2023 • edited

jalopezg-git commented Apr 17, 2023

pcanal commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023 • edited

vgvassilev commented Apr 17, 2023

vgvassilev commented Apr 17, 2023

jalopezg-git commented Apr 17, 2023

jalopezg-git commented Apr 18, 2023 • edited

jalopezg-git commented Apr 19, 2023

phsft-bot commented Apr 19, 2023

hahnjo left a comment

Choose a reason for hiding this comment

jalopezg-git commented Apr 13, 2023 •

edited

jalopezg-git commented Apr 13, 2023 •

edited

pcanal commented Apr 15, 2023 •

edited

pcanal commented Apr 15, 2023 •

edited

jalopezg-git commented Apr 17, 2023 •

edited

jalopezg-git commented Apr 18, 2023 •

edited