build: Add linter checking for accidental introduction of locale dependence #13041

This linter will check for code accidentally introducing locale dependencies.

Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. We should avoid using locale dependent functions if possible.

Context: #12881 (comment)

Example output:

$ contrib/devtools/lint-locale-dependence.sh
The locale dependent function tolower(...) appears to be used:
src/init.cpp:    if (s[0] == '0' && std::tolower(s[1]) == 'x') {

Unnecessary locale dependence can cause bugs that are very
tricky to isolate and fix. Please avoid using locale dependent
functions if possible.

Advice not applicable in this specific case? Add an exception
by updating the ignore list in contrib/devtools/lint-locale-dependence.sh

Note to reviewers: What is the most appropriate LOCALE_DEPENDENT_FUNCTIONS function list? What should be added or removed?

@laanwj Do you think the LOCALE_DEPENDENT_FUNCTIONS list is correct? :-)

Concept ACK. I think this is a good idea.

@laanwj Do you think the LOCALE_DEPENDENT_FUNCTIONS list is correct? :-)

It's a good start. Please add strftime too, that's the one that caused recent discussion in #12973.

@laanwj strftime and strptime added. Please re-review :-)

@laanwj Updated version with the known violations list split over multiple lines. Please re-review :-)

Candidates for inclusion in the list of locale dependent functions:

$ nm /usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so | grep -E '_l$' | \
      cut -f3 -d' ' | grep -E '^[a-z]' | sort | sed 's/_l$//g'
isalnum
isalpha
isblank
iscntrl
isdigit
isgraph
islower
isprint
ispunct
isspace
isupper
iswalnum
iswalpha
iswblank
iswcntrl
iswctype
iswdigit
iswgraph
iswlower
iswprint
iswpunct
iswspace
iswupper
iswxdigit
isxdigit
nl_langinfo
strcasecmp
strcoll
strerror
strfmon
strftime
strncasecmp
strptime
strtod
strtof
strtold
strtol
strtoll
strtoul
strtoull
strxfrm
tolower
toupper
towctrans
towlower
towupper
wcscasecmp
wcscoll
wcsftime
wcsncasecmp
wcstod
wcstof
wcstold
wcstol
wcstoll
wcstoul
wcstoull
wcsxfrm
wctrans
wctype

The use of strtoul(...) and strtoull(...) might be problematic too?

Updated: Added more functions to LOCALE_DEPENDENT_FUNCTIONS. Added two known violations for strtoul(...) and strtoull(...) in utilstrencodings.cpp.

Please re-review :-)

utACK 29b73b509f1aa537a835f9e75878622c3a18335f

@theuni @sipa Your language expertise is needed :-)

If you have time to review this PR – does LOCALE_DEPENDENT_FUNCTIONS look reasonable? :-)

Updated:

• Added more functions to LOCALE_DEPENDENT_FUNCTIONS.
• Added known atoi(…) violations for bitcoin-tx.cpp, init.cpp, qt/rpcconsole.cpp, torcontrol.cpp, util.cpp, utilstrencodings.cpp and utilstrencodings.h.
• Added known *printf(…) violations for dbwrapper.cpp, test/dbwrapper_tests.cpp and util.cpp.
• Now invoking git grep only once in order to minimize run-time.

Please re-review :-)

I think printf and fprintf(stderr, ... are fine. They are only used to print error messages, it's not important if they are formatted slightly different according to the locale. (not so much for sprintf vsnprintf etc which should be replaced with use of tinyformat, these end up in strings and might end up on the RPC interface or in files meant to be portable)

@laanwj You saw that fprintf(stderr, … is explicitly filtered out via grep -vE 'fprintf\(.*(stdout|stderr)' in the script? :-)

Should I keep fprintf (with the exclusion above) and remove printf? Makes sense?

@laanwj You saw that fprintf(stderr, … is explicitly filtered out via grep -vE 'fprintf(.*(stdout|stderr)' in the script? :-)

Oh that's good! No, hadn't noticed that yet.

Should I keep fprintf (with the exclusion above) and remove printf? Makes sense?

Yes I think so!

@laanwj Updated version with printf removed! Please re-review :-)

Thinking of it, it would make sense to add this list to the developer notes as well (next to the mention of the locale risks) , so it doesn't come out of the blue.

@laanwj Good point! Added a reference to the script and included the list in the developer notes. Looks good? :-)

Updated: Added some locale dependent Boost functions and the corresponding KNOWN_VIOLATIONS entries.

Added known violations:

• is_digit is used in src/core_read.cpp
• to_lower is used in src/netbase.cpp
• to_upper is used in src/rpc/server.cpp
• trim is used in src/core_read.cpp
• trim is used in src/httprpc.cpp
• trim_right is used in src/bitcoin-tx.cpp

Added Boost functions:

fold_case
is_digit
is_space
normalize
to_lower
to_title
to_upper
trim
trim_left
trim_right

@laanwj, is this PR getting ready for merge? :-) When it is merged my plan is to start replacing calls to locale dependent functions with their non-locale dependent counterparts.

@Empact Would you mind reviewing this PR? :-)

utACK f947ef02f9c8eee5957bb02e12cee4edd636c9fa

On Jun 5, 2018 11:15 AM, "Wladimir J. van der Laan" < notifications@github.com> wrote:

utACK f947ef0 https://github.com/bitcoin/bitcoin/commit/f947ef02f9c8eee5957bb02e12cee4edd636c9fa

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13041#issuecomment-394808844, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfpiNwEhc18PlReCb8tP5iuXoUiYLn6ks5t5sqtgaJpZM4TdXns .

Need to move contrib/devtools/lint-locale-dependence.sh to test/lint/lint-locale-dependence.sh

@ken2812221 Thanks! Moved! @laanwj @cnavigato Please re-review :-)

utACK 698cfd081144845f6246171b8a2a0cfa8daaecdb