Bug 130924 - replace debugging printf calls with SAL_INFO/SAL_WARN
Summary: replace debugging printf calls with SAL_INFO/SAL_WARN
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard: target:7.0.0 target:7.3.0 target:7.4.0
Keywords: difficultyMedium, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2020-02-25 10:49 UTC by Michael Stahl (allotropia)
Modified: 2022-04-06 06:29 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Stahl (allotropia) 2020-02-25 10:49:01 UTC
git grep "\(\b\|f\)printf"

shows that there about 3100 calls to various printf functions.

A substantial part of these are debugging output from C++ source files;
those should be replaced with SAL_INFO typically.

There is also a substantial part which should *not* be replaced:
* printf calls in shell scripts and Makefiles and AWK and Perl
* C source files
* ancient code in */test and */workben directories
* printf that are not for debugging but printing help messages for the end user
* printf that are not for debugging but producing output from some tool
* printf in external/ and odk/ and onlineupdate/source/libmar/
* printf in sal/ (there are bootstrapping problems here)

To a first approximation, a good heuristic is that a printf should be replaced if it's inside a #ifdef OSL_DEBUG_LEVEL or #ifdef DBGUTIL or similar.

For an example commit see
https://gerrit.libreoffice.org/plugins/gitiles/core/+/a8e83ce1d86892061603159f5c4460803d077fd5%5E!/

The first argument to SAL_INFO is a "log area", see 
include/sal/log-areas.dox for existing ones; it may be necessary to add new log areas there.
Comment 1 Stephan Bergmann 2020-02-25 11:00:33 UTC
...and see <https://wiki.documentfoundation.org/Development/GeneralProgrammingGuidelines#Assertions_and_Logging> for a general discussion when to use SAL_INFO and SAL_WARN
Comment 3 Yukio Siraichi 2020-03-19 07:37:10 UTC
I have been working on this issue for a while now. Since there are many files to modify, I uploaded a WIP on gerrit: https://gerrit.libreoffice.org/c/core/+/90729.

There are a couple of things that I am not sure how they are supposed to be implemented. Mainly things like line 336 in "bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx". There are a couple of for-loops for printing some debugging information like:

for (...) fprintf(stderr, "..., ", ...);

Which I turned into:

std::ostringstream oss;
for (...) oss << "..." << ... << ", ";
SAL_INFO(/* area */, oss);

Should I do something else?
Comment 4 Stephan Bergmann 2020-03-19 07:59:43 UTC
(In reply to Yukio Siraichi from comment #3)
> There are a couple of things that I am not sure how they are supposed to be
> implemented. Mainly things like line 336 in
> "bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx". There are a couple of
> for-loops for printing some debugging information like:

Especially for an easy hack, please only change code that you can build locally.  bridges/source/cpp_uno/gcc3_linux_alpha/cpp2uno.cxx is only built when building for Alpha CPUs on Linux (see bridges/Library_cpp_uno.mk), so it is unlikely that your local build would actually build that code.  Better leave such code alone then.
Comment 5 Stephan Bergmann 2020-03-19 08:30:05 UTC
For printf in test code, please note <https://gerrit.libreoffice.org/c/core/+/90729/1#message-fb09c15ab1b2231d2f2df2d8e22fcb748264234f>:  "For test code like basic/qa/cppunit/, I think turning output into SAL_INFO or SAL_WARN is not a good idea:  Test code should unconditionally report all relevant findings.  When a (CppUnit, JUnit, etc.) test succeeds, gbuild will normally hide all of the test's output anyway.  But when a test fails (esp. one executing on a tinderbox which you cannot debug directly, and for which all you have is the failed test's output), you want to always see all the relevant findings, regardless of the --enable-sal-log configuration and SAL_LOG env var settings."
Comment 6 Yukio Siraichi 2020-04-04 10:45:53 UTC
I think I am almost done. I have some questions on some of the files, though (see below).

- hwpfilter/source/formula.cxx
There are lots of 'debug' printf's, which more changes are needed for making it happen. The problem is that it is using a global var to keep track of indentation. One way to work around that is by making that variable a field of the 'Formula' class. What do you think?

- hwpfilter/source/grammar.cxx
- hwpfilter/source/lexer.cxx
Has some (at least they look like) 'yacc' code. Should the 'fprintf' commands be replaced here?

- idl/source/prj/database.cxx
- include/LibreOfficeKit/LibreOfficeKitInit.h
- jvmfwk/plugins/sunmajor/javaenvsetup/javaldx.cxx
- jvmfwk/plugins/sunmajor/pluginlib/sunjavaplugin.cxx
- vcl/osx/salinst.cxx
- vcl/source/uipreviewer/previewer.cxx
- vcl/unx/generic/app/i18n_cb.cxx
- vcl/unx/generic/app/saldata.cxx
The 'fprintf's here look like they are supposed to show up to some end-user (there are many other files that I am omitting here).

- oox/source/helper/propertymap.cxx
Data is being dumped in this file. It feels odd to replace them with SAL_* functions. Am I doing it right?

- sdext/source/pdfimport/xpdfwrapper/pdfioutdev_gpl.cxx
These are plain 'printf's. I feel like they should be replaced, but it just feels strange.

- sw/source/core/bastyp/calc.cxx
It is a 'printf' inside a 'main' function.

- unotools/source/i18n/localedatawrapper.cxx
It has a 'SAL_WARN' just after the 'fprintf's.

Finally, there are lots of 'printf's that use tabs or spaces for indenting debugging information. Should I leave them there? I feel that (at least when the data is not structured) there is no need for the indentation (not the case of 'hwpfilter/source/formula.cxx').
Comment 7 Yukio Siraichi 2020-04-11 01:57:14 UTC
ping
Comment 8 Xisco Faulí 2020-04-14 09:06:11 UTC
(In reply to Yukio Siraichi from comment #6)
> I think I am almost done. I have some questions on some of the files, though
> (see below).

Could you please submit the patch to gerrit? I think it's a better place to discuss technicalities than bugzilla
Comment 9 Michael Stahl (allotropia) 2020-04-14 10:21:03 UTC
(In reply to Yukio Siraichi from comment #6)
> I think I am almost done. I have some questions on some of the files, though
> (see below).
> 
> - hwpfilter/source/formula.cxx
> There are lots of 'debug' printf's, which more changes are needed for making
> it happen. The problem is that it is using a global var to keep track of
> indentation. One way to work around that is by making that variable a field
> of the 'Formula' class. What do you think?

hmm that's a bit odd, probably just replace the printf for a start and keep the macros around it?

> - hwpfilter/source/grammar.cxx
> - hwpfilter/source/lexer.cxx
> Has some (at least they look like) 'yacc' code. Should the 'fprintf'
> commands be replaced here?

probably this is generated code, better not replace there.

> - idl/source/prj/database.cxx
> - include/LibreOfficeKit/LibreOfficeKitInit.h
> - jvmfwk/plugins/sunmajor/javaenvsetup/javaldx.cxx
> - jvmfwk/plugins/sunmajor/pluginlib/sunjavaplugin.cxx
> - vcl/osx/salinst.cxx
> - vcl/source/uipreviewer/previewer.cxx
> - vcl/unx/generic/app/i18n_cb.cxx
> - vcl/unx/generic/app/saldata.cxx
> The 'fprintf's here look like they are supposed to show up to some end-user
> (there are many other files that I am omitting here).

agree, don't replace.

> - oox/source/helper/propertymap.cxx
> Data is being dumped in this file. It feels odd to replace them with SAL_*
> functions. Am I doing it right?

so there's definitely some code-generating printfs in the 2nd half of this file that shouldn't be replaced; it's not obvious to me what the #ifdef'd printfs in the 1st half are doing.

> - sdext/source/pdfimport/xpdfwrapper/pdfioutdev_gpl.cxx
> These are plain 'printf's. I feel like they should be replaced, but it just
> feels strange.

this is an exectuable that writes to stdout, don't replace.

> - sw/source/core/bastyp/calc.cxx
> It is a 'printf' inside a 'main' function.

that's some weird code, best don't touch.

> - unotools/source/i18n/localedatawrapper.cxx
> It has a 'SAL_WARN' just after the 'fprintf's.

that's weird, perhaps the author of that code could tell you why that is, that's Eike Rathke most likely.

> Finally, there are lots of 'printf's that use tabs or spaces for indenting
> debugging information. Should I leave them there? I feel that (at least when
> the data is not structured) there is no need for the indentation (not the
> case of 'hwpfilter/source/formula.cxx').

it might be helpful in some cases; the SAL_WARN are prefixed with a generated string but the indentation could just go behind the prefix.

PS: i see there's a patch in gerrit and it changes 1000 lines - that's a bit large to review, would it be possible to split that up into smaller chunks?
Comment 10 Commit Notification 2020-04-20 08:25:34 UTC
Yukio Siraichi committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/317c05a88f79096422681a23b61aa6ae9f3f084b

tdf#130924 [vcl] replace `*printf`s with `SAL_*` logging macros.

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Eike Rathke 2020-05-01 21:42:51 UTC
(In reply to Michael Stahl (CIB) from comment #9)
> > - unotools/source/i18n/localedatawrapper.cxx
> > It has a 'SAL_WARN' just after the 'fprintf's.
> 
> that's weird, perhaps the author of that code could tell you why that is,
> that's Eike Rathke most likely.
That code underwent some iterations.. the original was

    fprintf( stderr, "\n%s\n", pStr);
    fflush( stderr);
    DBG_ERROR( pStr);

that outputs the message on console (the function is called only in a debug build or if some environment variable is set and was meant to aid locale data developers to check their addition in a non-debug environment) and then wrote it to the debug utility that since long is gone, also because on Windows one never saw console output from a GUI program, don't know if that ever changed.

Next iteration (not by me) was

-    DBG_ERROR( pStr);
+    OSL_TRACE("%s", pStr);

probably just to get rid of DBG_ERROR().

Then followed

-    OSL_TRACE("%s", pStr);
+    SAL_WARN("unotools", pStr);

So that looks duplicated, but for the non-debug program run purpose if then the SAL_WARN() should be removed, not the fprintf()/fflush(). Iff Windows does actually display console output. Or so..
Comment 12 Xisco Faulí 2021-02-09 14:24:59 UTC
Dear Yukio Siraichi,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assign it back to yourself if you're still working on this.
Comment 13 Hossein 2021-08-16 10:40:19 UTC
You should try to choose between SAL_INFO and SAL_WARN based on the printed content, and the context. If you are about to print some INFORMATION, then choose SAL_INFO, and if it a WARNING then you should use SAL_WARN.

This is useful for deciding:
https://bugs.documentfoundation.org/show_bug.cgi?id=91794#c0

If not understandable from the context, you can choose the equivalent replacement:

-----
#if OSL_DEBUG_LEVEL > 2
fprintf(...)
#endif
-----
should become: SAL_INFO(...)

-----
#if OSL_DEBUG_LEVEL > 1
fprintf(...)
#endif
-----
should become: SAL_WARN(...)

-----
#if OSL_DEBUG_LEVEL > 0
fprintf(...)
#endif
-----
should become: SAL_WARN(...)

Please note that you should remove '#if OSL_DEBUG_LEVEL > x' and '#endif'.
Comment 14 Commit Notification 2021-08-16 14:21:04 UTC
Emircan Agac committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/617fc3a0ed28b0ccc7fa658e4ba13a0fc9fb353b

tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2021-08-16 20:18:03 UTC
Emircan Agac committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/42f536df5c5b15d34c7c5663c6653d5bedbc2dc2

tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 16 Commit Notification 2021-09-06 13:39:03 UTC
Emircan Agac committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/7a8ec6cddc9f37ba6ff1a98ad39702521c8fb36b

tdf#130924 : replace debugging printf calls with SAL_INFO/SAL_WARN

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Commit Notification 2022-03-05 07:36:11 UTC
pragat-pandya committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/6f121860d0537060084278da11842732a748d6b7

tdf#130924 replace debugging printf calls with SAL_INFO/SAL_WARN

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2022-04-06 06:29:53 UTC
offtkp committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/c9b69e412b9646f7d49f500d47a078c2ea155c8c

tdf#130924 replace debugging printf calls with SAL_INFO/SAL_WARN

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.