Add JIT & Interpreter build switches. #2031

gtrevi · 2023-02-04T01:44:58Z

Description

This PR adds two build switches for individually disabling the JIT compiler and the interpreter, as well as adding the corresponding support into code and project files.

Testing

CI/CD pipeline.

Documentation

GettingStarted.md

…witches

codecov · 2023-02-04T02:10:53Z

Codecov Report

Merging #2031 (91a8b37) into main (c4f8f96) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
+ Coverage   82.41%   82.66%   +0.25%     
==========================================
  Files         141      152      +11     
  Lines       25402    27546    +2144     
==========================================
+ Hits        20935    22771    +1836     
- Misses       4467     4775     +308

Impacted Files	Coverage Δ
libs/execution_context/ebpf_core.c	`92.25% <ø> (+1.62%)`	⬆️
tests/sample/ext/app/sample_ext_app.cpp	`98.59% <ø> (ø)`
libs/execution_context/ebpf_program.c	`82.40% <100.00%> (+1.87%)`	⬆️
tests/api_test/api_test.cpp	`97.54% <100.00%> (ø)`
ebpfapi/rpc_client.cpp	`56.14% <0.00%> (-1.01%)`	⬇️
tools/bpf2c/bpf_code_generator.h	`93.44% <0.00%> (ø)`
x64/Release/Elf.c	`0.00% <0.00%> (ø)`
x64/Release/EverParse.h	`0.00% <0.00%> (ø)`
tests/bpf2c_tests/test_helpers.h	`87.50% <0.00%> (ø)`
tools/bpf2c/bpf_code_generator.cpp	`96.67% <0.00%> (ø)`
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

docs/GettingStarted.md

cmake/options.cmake

saxena-anurag · 2023-02-05T04:57:05Z

tests/api_test/api_test.cpp

@@ -407,10 +413,12 @@ divide_by_zero_test_km(ebpf_execution_type_t execution_type)
    // If we don't bug-check, the test passed.
 }

+#if !defined(CONFIG_BPF_JIT_DISABLED)


Are we running the api_tests on 2 different flavors of the build (one which has JIT disabled and one which has JIT enabled)? If not, we will lose test coverage in kernel tests for JIT scenarios.

Currently, none of the testes use the regular_native-only build, which is the only using the new switches.
Coverage and more will be taken care of on #2030.

dthaler

Approving but see suggestion

dthaler · 2023-02-06T19:26:49Z

cmake/options.cmake

@@ -3,7 +3,8 @@

 option(EBPFFORWINDOWS_ENABLE_TESTS "Set to true to enable tests" true)
 option(EBPFFORWINDOWS_ENABLE_INSTALL "Set to true to enable the install target")
-option(EBPFFORWINDOWS_ENABLE_DISABLE_EBPF_INTERPRETER "Set to true to compile with the interpreter always disabled")
+option(EBPFFORWINDOWS_DISABLE_JIT "Set to true to compile with the JIT compiler disabled")
+option(EBPFFORWINDOWS_DISABLE_INTERPRETER "Set to true to compile with the Interpreter disabled")


Since the interpreter should be off by default, can we reverse the parity and require setting it to true to compile with the interpreter enabled?

@dthaler This was discussed internally, would alter the UX on Visual Studio, where you will have to build via CLI if you want the Interpreter enabled.

PS: should the interpreter is disabled by default in Release builds (i.e., regardless of the switch)?

saxena-anurag · 2023-02-06T19:36:21Z

libs/execution_context/kernel/execution_context_kernel.vcxproj

@@ -94,7 +94,7 @@
  </ItemDefinitionGroup>
  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
    <ClCompile>
-      <PreprocessorDefinitions>WINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP;WINAPI_PARTITION_DESKTOP=1;WINAPI_PARTITION_SYSTEM=1;WINAPI_PARTITION_APP=1;WINAPI_PARTITION_PC_APP=1;%(PreprocessorDefinitions);_NO_CRT_STDIO_INLINE=1;CONFIG_BPF_JIT_ALWAYS_ON=1</PreprocessorDefinitions>
+      <PreprocessorDefinitions>WINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP;WINAPI_PARTITION_DESKTOP=1;WINAPI_PARTITION_SYSTEM=1;WINAPI_PARTITION_APP=1;WINAPI_PARTITION_PC_APP=1;%(PreprocessorDefinitions);_NO_CRT_STDIO_INLINE=1;</PreprocessorDefinitions>


Should we add CONFIG_BPF_INTERPRETER_DISABLED since we are now removing CONFIG_BPF_JIT_ALWAYS_ON?

saxena-anurag · 2023-02-06T19:37:29Z

tests/api_test/api_test.vcxproj

@@ -67,7 +67,7 @@
  </ItemDefinitionGroup>
  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
    <ClCompile>
-      <PreprocessorDefinitions>NDEBUG;_CONSOLE;CONFIG_BPF_JIT_ALWAYS_ON=1;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Same question as above: Should we add CONFIG_BPF_INTERPRETER_DISABLED here?

Same as above, it's now moved to the Directory.Build.props for global control.

saxena-anurag · 2023-02-06T19:38:16Z

tests/sample/ext/app/sample_ext_app.vcxproj

@@ -67,7 +67,7 @@
  </ItemDefinitionGroup>
  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
    <ClCompile>
-      <PreprocessorDefinitions>NDEBUG;_CONSOLE;CONFIG_BPF_JIT_ALWAYS_ON=1;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>NDEBUG;_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>


same as above.

Same as above, it's now moved to the Directory.Build.props for global control.

gtrevi and others added 3 commits February 3, 2023 16:18

draft

5eeb3ff

Merge branch 'microsoft:main' into gtrevi/add-jit-interpreter-build-s…

dccf4dc

…witches

nit

ee46712

gtrevi requested review from dthaler, poornagmsft, Alan-Jowett, saxena-anurag, shankarseal, dv-msft and delaramamiri as code owners February 4, 2023 01:44

gtrevi changed the title ~~Gtrevi/add jit interpreter build switches~~ Add JIT & Interpreter build switches. Feb 4, 2023

dthaler reviewed Feb 4, 2023

View reviewed changes

docs/GettingStarted.md Show resolved Hide resolved

docs/GettingStarted.md Outdated Show resolved Hide resolved

gtrevi added 2 commits February 3, 2023 20:59

doc update

5cf76cd

doc update

28a3eb3

dthaler previously approved these changes Feb 4, 2023

View reviewed changes

saxena-anurag reviewed Feb 6, 2023

View reviewed changes

gtrevi and others added 2 commits February 6, 2023 10:53

Merge branch 'main' into gtrevi/add-jit-interpreter-build-switches

13ff8d3

description update

8b00011

gtrevi dismissed dthaler’s stale review via 8b00011 February 6, 2023 18:54

dthaler approved these changes Feb 6, 2023

View reviewed changes

saxena-anurag reviewed Feb 6, 2023

View reviewed changes

Alan-Jowett approved these changes Feb 6, 2023

View reviewed changes

Merge branch 'main' into gtrevi/add-jit-interpreter-build-switches

91a8b37

dthaler enabled auto-merge (squash) February 7, 2023 14:19

dthaler merged commit 694485a into microsoft:main Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JIT & Interpreter build switches. #2031

Add JIT & Interpreter build switches. #2031

gtrevi commented Feb 4, 2023

codecov bot commented Feb 4, 2023 •

edited

Loading

saxena-anurag Feb 5, 2023

gtrevi Feb 6, 2023

dthaler left a comment

dthaler Feb 6, 2023

gtrevi Feb 6, 2023

gtrevi Feb 7, 2023 •

edited

Loading

saxena-anurag Feb 6, 2023

saxena-anurag Feb 6, 2023

gtrevi Feb 6, 2023

saxena-anurag Feb 6, 2023

gtrevi Feb 6, 2023

Add JIT & Interpreter build switches. #2031

Add JIT & Interpreter build switches. #2031

Conversation

gtrevi commented Feb 4, 2023

Description

Testing

Documentation

codecov bot commented Feb 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtrevi Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2023 •

edited

Loading

gtrevi Feb 7, 2023 •

edited

Loading