Adopt clang-format for all files?

[derekbruening]

As part of adding lint style checks in #2369 we considered using clang-format but balked at reformatting every single file and thus adding a history hurdle. But as more contributions come in with style issues and as I see other projects successfully use clang-format to simplify things, I lean more and more toward biting the bullet and reformatting everything and then adding a pre-commit check.

[Carrotman42]

Is it possible to just clang-format new lines?

[derekbruening]

Is it possible to just clang-format new lines?

Possibly, but we are likely to have to tweak the existing style rules (because there may not be existing clang-format knobs to match everything), and we already have some older code violating things, and it's more complicated to set up editors and scripts to only format changes -- it seems simpler to get entire files in a consistent state. Xref other project experiences such as https://engineering.mongodb.com/post/succeeding-with-clangformat-part-2-the-big-reformat/

[fhahn]

Maybe we could make use of the clang-format-diff.py script: https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/clang-format-diff.py

It just formats the changed lines in a diff. To format a commit, I use something like

git diff -U0 --no-color HEAD^ | clang-format-diff.py -i -p1

With clang-format and clang-format-diff.py being in the PATH

[derekbruening]

FTR here are the settings I used a year ago though I did not finish tweaking them:

$ /work/llvm/build_rel/bin/clang-format -style=WebKit -dump-config > /tmp/clang-format-webkit
$ diff /tmp/clang-format-webkit .clang-format
5c5
< AlignAfterOpenBracket: DontAlign
---
> AlignAfterOpenBracket: Align
14c14
< AllowShortFunctionsOnASingleLine: All
---
> AllowShortFunctionsOnASingleLine: None
17,18c17,18
< AlwaysBreakAfterDefinitionReturnType: None
< AlwaysBreakAfterReturnType: None
---
> AlwaysBreakAfterDefinitionReturnType: All
> AlwaysBreakAfterReturnType: AllDefinitions
41c41
< ColumnLimit:     0
---
> ColumnLimit:     90
78c78
< PointerAlignment: Left
---
> PointerAlignment: Right
88c88
< SpacesInContainerLiterals: true
---
> SpacesInContainerLiterals: false
92c92
< Standard:        Cpp11
---
> Standard:        Cpp03

Cpp11 should be fine now.

One issue is clang versioning: it moves rapidly and always seems a pain to deal with, having to get the latest version all the time. AlwaysBreakAfterReturnType was not added until after at least 3.5. We might consider placing a clang-format binary in the repo: that is probably the best way to go to ensure all machines have the right version for our settings.

[derekbruening]

Starting to get into the final details here:

• Moving */ onto the next line: there is no clang-format support for this. ReflowComments will leave a comment's end style how it is but not change it. It may be easiest to change our style rule to allow */ on the end of the prior line.

• Indenting # include: the indent value has to be the general value. Again it may be easiest to change the style rule to indent that much rather than 1.

• Excluding long-line files (the list in make/style_checks/scripts/rules/line_length_content.tcl): we can wrap the files in // clang-format {off,on}. Maybe some of them we could abandon the exclusion: I'm thinking statsx.h, optionsx.h, instr_create.h, maybe more.

• Vera rules: some are still relevant even with clang-format: camel_case, multi_line_body, NOCHECKIN check, maybe carriage return and trailing space checks -- not certain about those.

• Running clang-format as part of CI: to avoid the overhead of downloading the 270MB LLVM binary release I still think checking in a single x86-64 clang-format binary just for Travis (and skipping on Appveyor as Travis should cover Windows code too) may be worth it -- or we can look into Travis saved images as IIRC there may be some support for saving some install bits.

[fhahn]

Running clang-format as part of CI: to avoid the overhead of downloading the 270MB LLVM binary release I still think checking in a single x86-64 clang-format binary just for Travis (and skipping on Appveyor as Travis should cover Windows code too) may be worth it -- or we can look into Travis saved images as IIRC there may be some support for saving some install bits.

http://apt.llvm.org/ provides apt repositories for various ubuntu versions (including Trusty) which provides clang-format packages (either clang-format-6.0 or clang-format-7.0 (current LLVM trunk)

[derekbruening]

I'm considering putting this in place this weekend. I think even if someone has a large outstanding diff it shouldn't be too painful to merge as it's all whitespace changes and clang-format will do the merge work for you.

[derekbruening]

I'm using clang-format 6.0 and am seeing a number of what look like bugs in clang-format as I don't see any competing rule that would cause this behavior.

Most of these are found by the vera++ checks, some by visual scanning the diff.

Here are some examples:

__cdecl confuses it?

-int __cdecl
-main(int argc, char *argv[], char *envp[])
+int __cdecl main(int argc, char *argv[], char *envp[])

Macro-generated-name confuses it:

-int
-TNAME(our_snprintf)(TCHAR *s, size_t max, const TCHAR *fmt, ...)
+int TNAME(our_snprintf)(TCHAR *s, size_t max, const TCHAR *fmt, ...)

Single-char operator name confuses it??

-bool
-analyzer_t::operator!()
+bool analyzer_t::operator!()

Greater-than-90-char lines:

              "x86_to_x64_private_bb_ibl",          "x86_to_x64_private_bb_ibl_template" },

#        define SET_FLAG(cc, flag) __asm__ __volatile__("set" #        cc " %0" : "=qm"(flag))

I tried increasing PenaltyExcessCharacter but it's already really high. I
don't get it: there are simple places to break.

[derekbruening]

I want to exclude some files from clang-format-diff.py without marking them
up with "// clang-format off":

• ext/drsyms/libefltc/include/*.h
• third_party/libgcc/*.c
• third_party/valgrind/*.h

I can't find any simple way to do it.

I see references online to a ".clang-format-ignore" file but it seem to be
ignored and it's not mentioned in the clang-format docs.

Maybe we could exclude those files from the diff generated to pass to
clang-format-diff.py but that gets more complex than I want to deal with now.

Going to punt for now. Maybe on changes to those the author temporarily
makes the format a non-FATAL_ERROR in the PR.

[derekbruening]

More clang-format bugs, including an interesting one: having a C++ token in C code seems to cause the parser to mess up.

Try formatting this .h file with IndentPPDirectives: AfterHash set and watch it fail to identify the include guard:

#ifndef _GLOBALS_SHARED_H_
#    define _GLOBALS_SHARED_H_ 1
#    define inline __inline
#endif /* _GLOBALS_SHARED_H_ */

Add an x and it works:

#ifndef _GLOBALS_SHARED_H_
#define _GLOBALS_SHARED_H_ 1
#define xinline __inline
#endif /* _GLOBALS_SHARED_H_ */

Ditto for define true (1).

These are correctly inside ifndef __cplusplus.

[derekbruening]

I'm about ready to put in the big change. Some things I'm not 100% happy with but overall I am pleased and I think this will improve readability and make contributing and reviewing simpler.

I'm pasting my exclusion notes. There is some overlap with prior comments above.

*** TODO put exclusions in place
**** DONE exclude syscall and decode tables => formatting statsx, optionsx, instr_create though
CLOSED: [2018-07-06 Fri 14:11]
- State "DONE" from "TODO" [2018-07-06 Fri 14:11]

I guess we should use the off-on blocks for things like line length in
decode_table.c?

Maybe abandon line-length exclusion for at least some of those files listed
in line_lenth_content.tcl?

Going through them:
Split lines:

• statsx.h
• optionsx.h
• instr_create.h (for ARM, will have to run through clang-format after re-gen)
  Don't split lines:
• syscallx.h
• decode_table.c
• table_a32_pred.c
• table_a32_unpred.c
• table_t32_16.c
• table_t32_16_it.c
• table_t32_base.c
• table_t32_coproc.c
• table_encode.c

**** DONE what about third_party/? => not formatting; want to ignore in travis but unclear how
CLOSED: [2018-07-06 Fri 14:11]
- State "DONE" from "TODO" [2018-07-06 Fri 14:11]

**** DONE what about ext/drsyms/libelftc/include/? => not formatting; want to ignore in travis but unclear how
CLOSED: [2018-07-06 Fri 20:19]
- State "DONE" from "TODO" [2018-07-06 Fri 20:19]
**** DONE style_test.c => excluding
CLOSED: [2018-07-06 Fri 20:19]
- State "DONE" from "TODO" [2018-07-06 Fri 20:19]
**** DONE what about ASM_CODE_ONLY? => live w/ it
CLOSED: [2018-07-06 Fri 14:11]
- State "DONE" from "TODO" [2018-07-06 Fri 14:11]

Avoid indenting for that? Not sure how w/o turning off formatting for
entire file, though.

**** DONE asm code inside .c files => exclude it
CLOSED: [2018-07-06 Fri 14:29]
- State "DONE" from "TODO" [2018-07-06 Fri 14:29]

Don't want this:

-        mov      REG_XAX, ARG1
+mov REG_XAX,
+    ARG1

I did:

$ for i in `git grep -c START_FILE | egrep -v ':0|.txt:|.cmake:|.asm:' | sed 's/:[0-9]//'`; do echo $i; sed -i -e '/^START_FILE/i \\/\/ clang-format off' $i; sed -i -e '/^END_FILE/a \\/\/ clang-format on' $i; done

**** DONE configure.cmake.h and dr_api.h ${var} => exclude it
CLOSED: [2018-07-06 Fri 14:35]
- State "DONE" from "TODO" [2018-07-06 Fri 14:35]

-#cmakedefine VERSION_NUMBER ${VERSION_NUMBER}
+#cmakedefine VERSION_NUMBER ${ VERSION_NUMBER }
=>
CMake Error at CMakeLists.txt:1205 (configure_file):
  Syntax error in cmake code when parsing string

    #define VERSION_NUMBER ${ VERSION_NUMBER }

  Invalid character (' ') in a variable name: ''

**** DONE mcxtx.h avoid indent? => exclude it
CLOSED: [2018-07-07 Sat 20:26]
- State "DONE" from "TODO" [2018-07-07 Sat 20:26]
**** DONE patchwork exported headers with weird indent requirements => fixed major things below; i#3092 covers the rest
CLOSED: [2018-07-07 Sat 20:29]
- State "DONE" from "TODO" [2018-07-07 Sat 20:29]

mcxtx.h is one example.
Another:

core/arch/instr_create_shared.h:/* (deliberately not indenting the #includes in API_EXPORT_ONLY for generated file) */

***** TODO (i#3092): [cleanup] refactor public interface headers and eliminate patchwork exports via genapi.pl
**** DONE header guard sometimes indents everything, sometimes not => clang-format bug; split globals_shared.h and globals.h C++ keyword defines into new c_defines.h to solve
CLOSED: [2018-07-07 Sat 20:26]
- State "DONE" from "TODO" [2018-07-07 Sat 20:26]

fragment.h: no indent
globals_shared.h: indent
globals.h: indent

Looks like just the two globals*.h fail to find the include guard.
OK, also these:
ext/drgui/drgui_tool_interface.h:# define DRGUI_TOOL_INTERFACE_H
suite/tests/tools.h:# define TOOLS_H

https://reviews.llvm.org/D35955

Preprocessor indentation also attempts to ignore include guards with the checks:
1. Include guards cover the entire file
2. Include guards don't have #else
3. Include guards begin with
#ifndef <var>
#define <var>

This seems to trigger it:

#    define inline __inline

So we have:

#ifndef _GLOBALS_SHARED_H_
#    define _GLOBALS_SHARED_H_ 1
#    define inline __inline
#endif /* _GLOBALS_SHARED_H_ */

But:

#ifndef _GLOBALS_SHARED_H_
#define _GLOBALS_SHARED_H_ 1
#define xinline __inline
#endif /* _GLOBALS_SHARED_H_ */

This also:

#        define true (1)

These are inside 'ifdndef __cplusplus', but the clang parser must be
treating "inline" and "true" as tokens and just mis-parsing them?

This is the one in globals.h:

#        define __try __try_forbidden_construct /* see case 4461 */

tools.h: "true" again. Fixed by including c_defines.h instead.

ext/drgui/drgui_tool_interface.h: small file: living w/ it.

***** DONE make a small c_defines.h to isolate
CLOSED: [2018-07-07 Sat 19:38]
- State "DONE" from "TODO" [2018-07-07 Sat 19:38]

**** DONE don't want to indent inside API_EXPORT_ONLY => genapi.pl removes it
CLOSED: [2018-07-07 Sat 20:00]
- State "DONE" from "TODO" [2018-07-07 Sat 20:00]

#define ZHEX64_FORMAT_STRING "%016" INT64_FORMAT "x"
#ifdef API_EXPORT_ONLY
#    define ZHEX32_FORMAT_STRING "%08x"

I have genapi.pl removing it.
Tested via:

$ pushd /work/dr/git/src/core && /usr/bin/perl /work/dr/git/src/core/lib/genapi.pl -core /work/dr/git/src/core -header /work/dr/git/build_x64_dbg_tests/include "-DINTERNAL -DDEBUG -DDRSTATS_DEMO -DX86 -DX64 -DLINUX -DUNIX -DCLIENT_INTERFACE -DAPP_EXPORTS -DKSTATS -DHAVE_FVISIBILITY -DHAVE_TYPELIMITS_CONTROL -DANNOTATIONS -DDR_DO_NOT_DEFINE_uint -DDR_DO_NOT_DEFINE_ushort -DDR__Bool_EXISTS -DASSEMBLE_WITH_GAS -DHAVE_MEMINFO -DHAVE_MEMINFO_MAPS -DHAVE_TLS -DHAVE_SIGALTSTACK -DUSE_VISIBILITY_ATTRIBUTES -DDYNAMORIO_IR_EXPORTS -DCUSTOM_TRACES -DCLIENT_SIDELINE -DDR_APP_EXPORTS -DDEBUG_MEMORY -DHEAP_ACCOUNTING -DDEADLOCK_AVOIDANCE -DSHARING_STUDY -DHASHTABLE_STATISTICS -DAPI_EXPORT_ONLY" && popd

And then looking at include/dr_defines.h.

**** DONE don't want to indent inside CLIENT_INTERFACE in instrument_api.h => genapi.pl removes it
CLOSED: [2018-07-07 Sat 20:25]
- State "DONE" from "TODO" [2018-07-07 Sat 20:25]

**** DONE DR_LOG_* indented => removed if 0 hack no longer needed
CLOSED: [2018-07-07 Sat 20:25]
- State "DONE" from "TODO" [2018-07-07 Sat 20:25]

**** TODO genapi.pl indent removal not perfect: some issues

In dr_ir_macros.h:

#ifdef X86
#include "dr_ir_macros_x86.h"
#elif defined(AARCH64)

Prob just going to live with it and let i#3092 clean it up.

**** DONE clang-format bug: no break after type for single-char operators => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

ipc_reader.cpp:

-bool
-analyzer_t::operator!()
+bool analyzer_t::operator!()

I tried
/extsw/pkgs/llvm/clang+llvm-6.0.0-x86_64-linux-gnu-Fedora27/bin/clang-format
in case my local 6.0-ish build is missing some fixes but I get the same result.

**** DONE clang-format bug: no break after type for __decl => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

This isn't right -- __cdecl confuses it??

-int __cdecl
-main(int argc, char *argv[], char *envp[])
+int __cdecl main(int argc, char *argv[], char *envp[])

**** DONE clang-format bug: no break after type for macro-wrapped func name => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

-int
-TNAME(our_snprintf)(TCHAR *s, size_t max, const TCHAR *fmt, ...)
+int TNAME(our_snprintf)(TCHAR *s, size_t max, const TCHAR *fmt, ...)

**** DONE clang-format bug: >90-char lines! => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

              "x86_to_x64_private_bb_ibl",          "x86_to_x64_private_bb_ibl_template" },

#        define SET_FLAG(cc, flag) __asm__ __volatile__("set" #        cc " %0" : "=qm"(flag))

Some are caused by comment alignment:

-    case TYPE_INDIR_VAR_XIREG: /* indirect reg varies by ss only, base is 4x8,
-                                * opsize varies by data16 except on 64-bit Intel */
+    case TYPE_INDIR_VAR_XIREG:        /* indirect reg varies by ss only, base is 4x8,
+                                       * opsize varies by data16 except on 64-bit Intel */

I tried increasing PenaltyExcessCharacter but it's already really high. I
don't get it: there are simple places to break.

**** DONE clang-format bug: mis-parses read_and_verify_dr_marker_common => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

Gets confused by braces in ifdefs and gets the indentation all wrong in the
tail end of the function.

**** DONE clang-format bug: turns define into macro func => excluded
CLOSED: [2018-07-06 Fri 22:31]
- State "DONE" from "TODO" [2018-07-06 Fri 22:31]

-#    define true  (1)
+#            define true(1)

/work/dr/git/src/core/lib/globals_shared.h:209:26: error: "1" may not appear in macro parameter list
 #            define true(1)

**** DONE clang-format bug: threw out macro line continuations
CLOSED: [2018-07-07 Sat 20:43]
- State "DONE" from "TODO" [2018-07-07 Sat 20:43]

Just for this macro in core/win32/eventlog.c:

-/* pass &p */                                           \
-#define VARFIELD(pp, pend, val, len) do {               \
-    if (!(*(pp)) || (*(pp)) + (len) > (pend))           \
-        return 0;                                       \
-    memcpy((*(pp)), (val), (len));                      \
-    *(pp) += (len);                                     \
-} while (0)
+/* pass &p */
+#define VARFIELD(pp, pend, val, len) do
+{
+    if (!(*(pp)) || (*(pp)) + (len) > (pend))
+        return 0;
+    memcpy((*(pp)), (val), (len));
+    *(pp) += (len);
+}
+while (0)

If I manually move the "do" to the next line it then works. ?!?

**** DONE L_EXPAND_LEVEL fails on multi-line strings
CLOSED: [2018-07-07 Sat 21:11]
- State "DONE" from "TODO" [2018-07-07 Sat 21:11]

FAILED: C:\PROGRA~2\MICROS~1.0\VC\bin\cl.exe   /nologo -DINCLUDE_EVENTS -DRC_IS_CORE -Ddynamorio_EXPORTS -ID:\derek\dr\git\src\core\arch\x86 -ID:\derek\dr\git\src\core\win32 -ID:\derek\dr\git\src\core\arch -ID:\derek\dr\git\src\core\lib -I. -Iinclude\annotations /nologo /MP /GF /FS /GS- /MT  /Zi /Od /W4 /WX     /Gs65536 /showIncludes /Focore\CMakeFiles\dynamorio.dir\win32\diagnost.c.obj /Fdcore\CMakeFiles\dynamorio.dir\ /FS -c D:\derek\dr\git\src\core\win32\diagnost.c
D:\derek\dr\git\src\core\win32\diagnost.c(1324) : error C2308: concatenating mismatched strings
        Concatenating wide "\Registry\Machine\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution " with narrow "Options"

It fits though: a holdover from the failed include guard, b/c clang-format
won't un-wrap.

**** DONE clang-format bug: messes up some inline asm => excluded
CLOSED: [2018-07-07 Sat 21:12]
- State "DONE" from "TODO" [2018-07-07 Sat 21:12]

It is nearly always correct but in hooker-ntdll.c and several other tests
it does this to this particular pattern:

-    __asm {
-        popf
-        popa
+    __asm
+    {
+        popf popa

[derekbruening]

I'm having trouble installing clang-format-6.0 on Travis. Whether I do this:

wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
sudo apt-get install -y software-properties-common
sudo apt-add-repository "deb http://apt.llvm.org/trusty/ llvm-toolchain-trusty-6.0 main"
sudo apt-get update
sudo apt-get install -y clang-format-6.0

Or this:

      addons:
        apt:
          sources:
          - llvm-toolchain-trusty-6.0
          packages:
          - clang-format-6.0

I hit this error:

The following packages have unmet dependencies:
 clang-format-6.0 : Depends: libllvm6.0 (= 1:6.0.1~svn334776-1~exp1~20180627160813.83) but it is not going to be installed
                    Depends: libstdc++6 (>= 4.9) but 4.8.4-2ubuntu1~14.04.4 is to be installed
E: Unable to correct problems, you have held broken packages.

What's weird is that searching github shows several other projects installing in one of these two ways onto Travis and succeeding. So far I can't tell what's different about their setup vs ours.

[fhahn]

Hm it's weird that the clang-format-6.0 package for trusty requires libstdc++6 (4.9), but it looks like trusty only ships version 4.8. Maybe installing gcc-4.9 from https://wiki.ubuntu.com/ToolChain#PPA_packages would do the trick?

[fhahn]

I just saw that http://apt.llvm.org/ recommends using deb http://ppa.launchpad.net/ubuntu-toolchain-r/test/ubuntu trusty main to get a suitable libstdc++ on trusty

[derekbruening]

Thanks, I did see another project using the source ubuntu-toolchain-r-test: I'll try that.

Mac is also broken: https://travis-ci.org/DynamoRIO/dynamorio/jobs/401354219. I will fix it.

[derekbruening]

I set up the format check as part of the 64-bit clang, so the final Travis build, to avoid adding more time to the ones running tests. A failure looks like this:

https://travis-ci.org/DynamoRIO/dynamorio/jobs/401568760

$ suite/runsuite_wrapper.pl travis $EXTRA_ARGS
Detected a Travis clang suite: disabling running of tests
-- Changes are not formatted properly:
--- core/lib/instrument.c	(before formatting)
+++ core/lib/instrument.c	(after formatting)
@@ -2727,7 +2727,7 @@
         case WINDOWS_VERSION_2003: info->version = DR_WINDOWS_VERSION_2003; break;
         case WINDOWS_VERSION_XP: info->version = DR_WINDOWS_VERSION_XP; break;
         case WINDOWS_VERSION_2000: info->version = DR_WINDOWS_VERSION_2000; break;
-          case WINDOWS_VERSION_NT: info->version = DR_WINDOWS_VERSION_NT; break;
+        case WINDOWS_VERSION_NT: info->version = DR_WINDOWS_VERSION_NT; break;
         default: CLIENT_ASSERT(false, "unsupported windows version");
         };
     } else
CMake Error at /home/travis/build/DynamoRIO/dynamorio/suite/runsuite.cmake:218 (message):
  FATAL ERROR: Changes are not formatted properly (see diff above)!

[derekbruening]

Running the checks only in the one clang build means you can see build failures simultaneously, w/o all builds being blocked up front w/ format errors.