Generate compile_commands.json on macOS Builds

[Brandon]

Add support to generate compile_commands.json on macOS builds.

[Brandon]

Created attachment 450103 [details]
patch

[Saam Barati]

Comment on attachment 450103 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450103&action=review

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12762
> +				OTHER_CPLUSPLUSFLAGS = (

can this be put in the various xcconfig files? I think that might be the more canonical way of doing it, but not 100% sure.

[Brandon]

(In reply to Saam Barati from comment #2)
> Comment on attachment 450103 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450103&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12762
> > +				OTHER_CPLUSPLUSFLAGS = (
> 
> can this be put in the various xcconfig files? I think that might be the
> more canonical way of doing it, but not 100% sure.

I edited this parameter using Xcode. 
I will give just editing the .xcconfig file a try.

[Elliott Williams]

Comment on attachment 450103 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450103&action=review

+1 to moving all settings to xcconfigs. JavaScriptCore.xcconfig should be the one you need.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12765
> +					"-isystem",
> +					"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",

I think this include is redundant. JSC already sets it in Source/JavaScriptCore/Configurations/Base.xcconfig, so it should get picked up by `$(inherited)`.

[Alexey Proskuryakov]

Comment on attachment 450103 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450103&action=review

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12766
> +					"-gen-cdb-fragment-path",

I did a quick web search for this and for clang-scan-deps, but I still don't quite understand the overall goal. Do you need this generated always, or just in some specific build configuration?

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12767
> +					../../WebKitBuild/compile_commands,

"../../WebKitBuild" won't exist universally. This will either break the build, or produce very unexpected artifacts.

[Saam Barati]

(In reply to Elliott Williams from comment #4)
> Comment on attachment 450103 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450103&action=review
> 
> +1 to moving all settings to xcconfigs. JavaScriptCore.xcconfig should be
> the one you need.

We don't want Base.xcconfig so it's picked up by all targets?

> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12765
> > +					"-isystem",
> > +					"$(SDKROOT)/System/Library/Frameworks/System.framework/PrivateHeaders",
> 
> I think this include is redundant. JSC already sets it in
> Source/JavaScriptCore/Configurations/Base.xcconfig, so it should get picked
> up by `$(inherited)`.

[Elliott Williams]

(In reply to Saam Barati from comment #6)
> (In reply to Elliott Williams from comment #4)
> > Comment on attachment 450103 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=450103&action=review
> > 
> > +1 to moving all settings to xcconfigs. JavaScriptCore.xcconfig should be
> > the one you need.
> 
> We don't want Base.xcconfig so it's picked up by all targets?

You're right -- I didn't realize these were project-level settings. (And like Alexey, I don't understand the overall goal here :P)

[Brandon]

Some background information on the patch:

Getting code completion, compile errors, refactoring, and go-to definition for WebKit can be tricky to setup properly in editors like Emacs or Vim. clangd [0] is a tool that can help to solve these problems and works with a wide range of editors. 

clangd works by looking at the compile_commands.json file, which this patch generates. This file contains all the build commands and arguments that are used during compilation, so clangd can smartly index the files.

Xcode does not autogenerate the compile_commands.json, so I was doing it at build time instead.

[0]: https://clangd.llvm.org/features

[Brandon]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 450103 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450103&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12766
> > +					"-gen-cdb-fragment-path",
> 
> I did a quick web search for this and for clang-scan-deps, but I still don't
> quite understand the overall goal. Do you need this generated always, or
> just in some specific build configuration?
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12767
> > +					../../WebKitBuild/compile_commands,
> 
> "../../WebKitBuild" won't exist universally. This will either break the
> build, or produce very unexpected artifacts.

It would be nice to do this for every build as it should not negatively impact the build time, and only generates a few mb total of information.

[Elliott Williams]

Thank you for the explanation :)

I could see an argument that this is not a workflow we support in the Xcode build, and therefore you should do this locally with your build setting overrides, or with a command line invocation like

    make ... ARGS='OTHER_CPLUSPLUSFLAGS="$(inherited) -gen-cdb-fragment-path $(PWD)/compile-commands"'

That said, if there's no meaningful cost to producing these, and it helps everyone who's not editing in Xcode, it seems like a meaningful enhancement to me. Alexey, do you feel the same way?

[Saam Barati]

(In reply to Elliott Williams from comment #10)
> Thank you for the explanation :)
> 
> I could see an argument that this is not a workflow we support in the Xcode
> build, and therefore you should do this locally with your build setting
> overrides, or with a command line invocation like
> 
>     make ... ARGS='OTHER_CPLUSPLUSFLAGS="$(inherited) -gen-cdb-fragment-path
> $(PWD)/compile-commands"'
> 
> That said, if there's no meaningful cost to producing these, and it helps
> everyone who's not editing in Xcode, it seems like a meaningful enhancement
> to me. Alexey, do you feel the same way?

I've been giving Brandon's original patch a test run. It's been super helpful to me.

I do think it's meaningful to improve the xcodebuild for folks not actually using the IDE.

If we do decide to do it by default, I think we should measure impact on compile time first.

[Alexey Proskuryakov]

This should probably be added to DebugRelease.xcconfig, not to Base.xcconfig, to avoid unnecessarily changing production builds.

Putting the directory to the root of WebKitBuild is also not great because it's a location shared between all build styles and platforms - there is a reason why everything goes into subdirectories like Release-iphonesimulator there. But can clangd deal with switching between build styles?

I found a guide using cdb where it's recommended to disable precompiled headers to make it work better, <https://www.barebones.com/support/bbedit/lsp-notes.html#caveats>. This of course would be unacceptable to do when generating it as part of normal builds. Plus there are some other flags which I'm not sure if they change anything for WebKit. How bad is the impact of generating cdb with precompiled headers?

> If we do decide to do it by default, I think we should measure impact on compile time first.

Agreed.

[Saam Barati]

(In reply to Alexey Proskuryakov from comment #12)
> This should probably be added to DebugRelease.xcconfig, not to
> Base.xcconfig, to avoid unnecessarily changing production builds.
Yeah, definitely. I was wrong to say Base.xcconfig at first. I talked to Brandon on slack and we decided on putting it inside DebugRelease to avoid generating during production builds.

That said, I think Brandon might go with an approach where it's just put inside LocalOverrides.xcconfig and then it can be opt in via that.

> 
> Putting the directory to the root of WebKitBuild is also not great because
> it's a location shared between all build styles and platforms - there is a
> reason why everything goes into subdirectories like Release-iphonesimulator
> there. But can clangd deal with switching between build styles?
> 
> I found a guide using cdb where it's recommended to disable precompiled
> headers to make it work better,
> <https://www.barebones.com/support/bbedit/lsp-notes.html#caveats>. This of
> course would be unacceptable to do when generating it as part of normal
> builds. Plus there are some other flags which I'm not sure if they change
> anything for WebKit. How bad is the impact of generating cdb with
> precompiled headers?
> 
> > If we do decide to do it by default, I think we should measure impact on compile time first.
> 
> Agreed.

[Brandon]

Created attachment 450587 [details]
Patch

[Elliott Williams]

Comment on attachment 450587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450587&action=review

Have you considered implementing this logic in our Makefiles? (probably Makefile.shared)

I think it'd be possible to have a flag like `make d EXPORT_COMPILE_COMMANDS=YES` which would build with the appropriate compiler flags, then merge them together to produce a final compile_commands.json. There's even precedent for reading Make variables from user defaults, so you wouldn't have to remember to pass the flag every build: https://github.com/WebKit/WebKit/blob/fde694156283fc02d22a32e3d127b3285f424460/Makefile.shared#L50-L51

I don't want to derail, since I know you've been iterating on this already, but I think that tying things together in Make would be a UX improvement.

> Tools/Scripts/generate-compile-commands.sh:29
> +# Script to generate compile_commands.json file

High level thought: Could this be a wiki page? i.e. “To generate compiler metadata for VSCode, add these lines to your LocalOverrides.xcconfig”.

In general, I think this kind of thing is easy to express in documentation, and we ought to have some documentation for building with non-Xcode editors on Mac, so that folks other than us know that this functionality is availiable :)

> Tools/Scripts/setup-compile-commands-configuration.sh:35
> +echo "WARNING_CFLAGS = \"-gen-cdb-fragment-path\" \"/tmp/compile_commands\" \$(inherited)

nit: WARNING_CFLAGS should probably be OTHER_CFLAGS, since these flags don't relate to warnings.

[Saam Barati]

Comment on attachment 450587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450587&action=review

> Tools/ChangeLog:8
> +        Add support to JavaScriptCore for generating compile_commands.json.

just JavaScriptCore?

> Tools/Scripts/generate-compile-commands.sh:35
> +if ! [ -d /tmp/compile_commands/ ]; then

Why look in here? What if I have multiple builds happening simultaneously? Why isn't this just part of my Debug/Release build directory? That seems like the most logical place to put this stuff. Then this script can take an option for "--release" and "--debug", pick release or debug automatically if there is only one type of build.

> Tools/Scripts/generate-compile-commands.sh:61
> +echo "Generated compile_commands.json"

can this print the path of where it gets generated? What're we doing for debug vs release?

> Tools/Scripts/setup-compile-commands-configuration.sh:41
> +echo "Succesfully generated Config files"
> +echo "Configuration is stored in $(dirname "$0")/../../../LocalOverrides.xcconfig"

Is there nothing we can hook into with xcodebuild to automatically run generate-compile-commands.sh when we're done building? Maybe we should add an argument to "make"?

[Brandon]

(In reply to Elliott Williams from comment #15)
> Comment on attachment 450587 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450587&action=review
> 
> Have you considered implementing this logic in our Makefiles? (probably
> Makefile.shared)
> 
> I think it'd be possible to have a flag like `make d
> EXPORT_COMPILE_COMMANDS=YES` which would build with the appropriate compiler
> flags, then merge them together to produce a final compile_commands.json.
> There's even precedent for reading Make variables from user defaults, so you
> wouldn't have to remember to pass the flag every build:
> https://github.com/WebKit/WebKit/blob/
> fde694156283fc02d22a32e3d127b3285f424460/Makefile.shared#L50-L51
> 
> I don't want to derail, since I know you've been iterating on this already,
> but I think that tying things together in Make would be a UX improvement.
>

You are absolutely right! Modifying make to support something like EXPORT_COMPILE_COMMANDS=YES would be a significant improvement over my proposed method.

 
> > Tools/Scripts/generate-compile-commands.sh:29
> > +# Script to generate compile_commands.json file
> 
> High level thought: Could this be a wiki page? i.e. “To generate compiler
> metadata for VSCode, add these lines to your LocalOverrides.xcconfig”.
> 
> In general, I think this kind of thing is easy to express in documentation,
> and we ought to have some documentation for building with non-Xcode editors
> on Mac, so that folks other than us know that this functionality is
> availiable :)
> 

A wiki page or an entry on the website is definitely needed. I was just waiting to merge the implementation before writing one up 🙂.

> > Tools/Scripts/setup-compile-commands-configuration.sh:35
> > +echo "WARNING_CFLAGS = \"-gen-cdb-fragment-path\" \"/tmp/compile_commands\" \$(inherited)
> 
> nit: WARNING_CFLAGS should probably be OTHER_CFLAGS, since these flags don't
> relate to warnings.

[Brandon]

(In reply to Saam Barati from comment #16)
> Comment on attachment 450587 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450587&action=review
> 
> > Tools/ChangeLog:8
> > +        Add support to JavaScriptCore for generating compile_commands.json.
> 
> just JavaScriptCore?
> 

I didn't catch that mistake until after I posted it.
This supports all WebKit projects.

> > Tools/Scripts/generate-compile-commands.sh:35
> > +if ! [ -d /tmp/compile_commands/ ]; then
> 
> Why look in here? What if I have multiple builds happening simultaneously?
> Why isn't this just part of my Debug/Release build directory? That seems
> like the most logical place to put this stuff. Then this script can take an
> option for "--release" and "--debug", pick release or debug automatically if
> there is only one type of build.
> 

My method does not address multiple build scenarios well. Moving it back to the build directory would be more optimal. 

> > Tools/Scripts/generate-compile-commands.sh:61
> > +echo "Generated compile_commands.json"
> 
> can this print the path of where it gets generated? What're we doing for
> debug vs release?
> 

Sure.


> > Tools/Scripts/setup-compile-commands-configuration.sh:41
> > +echo "Succesfully generated Config files"
> > +echo "Configuration is stored in $(dirname "$0")/../../../LocalOverrides.xcconfig"
> 
> Is there nothing we can hook into with xcodebuild to automatically run
> generate-compile-commands.sh when we're done building? Maybe we should add
> an argument to "make"?

Adding support in make should definitely allows us to run the script.

[Radar WebKit Bug Importer]

<rdar://problem/88416409>

[Brandon]

Created attachment 451810 [details]
WIP Patch

[Elliott Williams]

Comment on attachment 451810 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451810&action=review

This is relevant to each project's DebugRelease.xcconfig changes:

> PerformanceTests/DecoderTest/Configurations/DebugRelease.xcconfig:41
> +OTHER_CFLAGS = $(COMPILE_COMMANDS_$(EXPORT_COMPILE_COMMANDS));
> +OTHER_CPLUSPLUSFLAGS = $(COMPILE_COMMANDS_$(EXPORT_COMPILE_COMMANDS));
> +COMPILE_COMMANDS_ = $(inherited);
> +COMPILE_COMMANDS_YES = $(inherited) "-gen-cdb-fragment-path" '$(BUILT_PRODUCTS_DIR)/compile_commands';

- I think OTHER_CFLAGS and OTHER_CPLUSPLUSFLAGS need an $(inherited), to pick up settings defined in each project's Base.xcconfig. This may be why you're seeing other compiler flags get clobbered.
- The COMPILE_COMMANDS_* variables shouldn't need to use $(inherited).  There aren't lower layers of build settings setting which define the COMPILE_COMMANDS variables, so it doesn't seem like it would ever expand to anything. 
- You should only need to set OTHER_CFLAGS. I believe that Xcode passes that setting to clang in C _and_ C++ modes.

[Brandon]

Created attachment 452272 [details]
WIP Patch

[Brandon]

Created attachment 452291 [details]
patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Brandon]

Created attachment 452293 [details]
patch

[Brandon]

Created attachment 452313 [details]
patch

[Elliott Williams]

Comment on attachment 452313 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452313&action=review

Some small nits, but overall I'm glad to see this is coming together!

If you are still thinking about how to automate that last step (running Tools/Scripts/generate-compile-commands), I think this'll work inside the Makefile:

    debug d: force
    	@$(call set_webkit_configuration,--debug)
    	@$(call invoke_xcode,,,GCC_PREPROCESSOR_DEFINITIONS='$(GCC_PREPROCESSOR_ADDITIONS) $$(inherited)')
    	$(SCRIPTS_PATH)/generate-compile-commands $(shell perl -I$(SCRIPTS_PATH) -Mwebkitdirs -e 'setConfiguration(); print productDir()' -- $(SDKROOT:%=--sdk %) --debug)

and do the same for the `release` recipe, but pass `--release` :)

> Makefile.shared:56
> +ifeq (, $(findstring EXPORT_COMPILE_COMMANDS, $(ARGS)))
> +		XCODE_OPTIONS += EXPORT_COMPILE_COMMANDS=YES;
> +		XCODE_OPTIONS += GCC_PRECOMPILE_PREFIX_HEADER=NO;
> +		XCODE_OPTIONS += CLANG_ENABLE_MODULE_DEBUGGING=NO;
> +		XCODE_OPTIONS += COMPILER_INDEX_STORE_ENABLE=NO;
> +endif
> +

I think you should pass EXPORT_COMPILE_COMMANDS directly to Make, not via ARGS, because it has Make-specific logic and isn't _just_ an xcode build setting.

```
ifneq (,$(EXPORT_COMPILE_COMMANDS))
    XCODE_OPTIONS += ...
endif
```

This also makes the UX a bit nicer, since you'll type `make EXPORT_COMPILE_COMMANDS=YES` instead of `make ARGS=EXPORT_COMPILE_COMMANDS=YES`.

> PerformanceTests/DecoderTest/Configurations/DebugRelease.xcconfig:39
> +COMPILE_COMMANDS_ = "";

This setting isn't necessary: xcconfig variables are empty by default, so it's fine to let OTHER_CFLAGS expand $(COMPILE_COMMANDS_) without us defining it. Also, xcconfig doesn't do string quoting, so "" are parsed as literal quote marks here.

> Tools/Scripts/generate-compile-commands:1
> +#!/bin/sh

Use `#!/bin/sh -e` to exit if there's an error inside the script.

[Brandon]

Created attachment 452455 [details]
patch

[Brandon]

Created attachment 452456 [details]
patch

[Saam Barati]

Comment on attachment 452456 [details]
patch

r=me

[EWS]

Committed r290149 (247488@main): <https://commits.webkit.org/247488@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452456 [details].

[Robert Jenner]

Reverted r290149 for reason:

Broke Debug Builds.

Committed r290179 (247509@trunk): <https://commits.webkit.org/247509@trunk>

[Brandon]

Created attachment 452685 [details]
patch

[EWS]

Committed r290227 (247553@main): <https://commits.webkit.org/247553@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452685 [details].

[Elliott Williams]

Comment on attachment 452685 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452685&action=review

Looks great! r=me after this scripting change:

> Tools/Scripts/generate-compile-commands:48
> +# Check compile_commands folder exists
> +compile_commands_dir = sys.argv[1] + "/compile_commands/"
> +
> +if not os.path.exists(compile_commands_dir):
> +    print("Please specify the build directory with compile_commands.")
> +    print("Example: generate-compile-commands WebKitBuild/Release")
> +    exit(1)

This will crash if the user doesn't pass argv[1]. Since this is a python script now, can we use ArgumentParser? This will also give us `-h|--help` flags for free :)

Xcode calls this directory BUILT_PRODUCTS_DIR. Can we name it the same here for consistency?

```python
import argparse
parser = argparse.ArgumentParser()
parser.add('built_products_dir', help='path to directory containing generated compile commands (ex: WebKitBuild/Release)')
opts = parser.parse()

# use it:
compile_commands_dir = opts.built_products_dir + '/compile_commands/'
```