From 387f894dbd8b627e2547b6d4a83d3daf8fb1edce Mon Sep 17 00:00:00 2001 From: Gabriel Terwesten Date: Wed, 6 Sep 2023 14:36:41 +0200 Subject: [PATCH] [native_toolchain_c] Default handling for PIC/PIE compiler flags (#121) --- pkgs/native_toolchain_c/CHANGELOG.md | 5 + .../lib/src/cbuilder/cbuilder.dart | 20 +- .../lib/src/cbuilder/run_cbuilder.dart | 18 +- pkgs/native_toolchain_c/pubspec.yaml | 2 +- .../cbuilder/cbuilder_cross_android_test.dart | 4 +- .../cbuilder/cbuilder_cross_ios_test.dart | 2 +- .../cbuilder_cross_linux_host_test.dart | 2 +- .../cbuilder_cross_macos_host_test.dart | 2 +- .../cbuilder_cross_windows_host_test.dart | 2 +- .../test/cbuilder/cbuilder_test.dart | 248 +++++++++++------- pkgs/native_toolchain_c/test/helpers.dart | 26 ++ 11 files changed, 224 insertions(+), 107 deletions(-) diff --git a/pkgs/native_toolchain_c/CHANGELOG.md b/pkgs/native_toolchain_c/CHANGELOG.md index e62a5d701..0f8852295 100644 --- a/pkgs/native_toolchain_c/CHANGELOG.md +++ b/pkgs/native_toolchain_c/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.2.2 + +- Generate position independent code for libraries by default and add + `pic` option to control this behavior. + ## 0.2.1 - Added `defines` for specifying custom defines. diff --git a/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart b/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart index fa3313dd2..ab43adb69 100644 --- a/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart +++ b/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart @@ -81,6 +81,20 @@ class CBuilder implements Builder { /// Defaults to `true`. final bool ndebugDefine; + /// Whether the compiler will emit position independent code. + /// + /// When set to `true`, libraries will be compiled with `-fPIC` and + /// executables with `-fPIE`. Accordingly the corresponding parameter of the + /// [executable] constructor is named `pie`. + /// + /// When set to `null`, the default behavior of the compiler will be used. + /// + /// This option has no effect when building for Windows, where generation of + /// position independent code is not configurable. + /// + /// Defaults to `true` for libraries and `false` for executables. + final bool? pic; + CBuilder.library({ required this.name, required this.assetId, @@ -90,6 +104,7 @@ class CBuilder implements Builder { this.defines = const {}, this.buildModeDefine = true, this.ndebugDefine = true, + this.pic = true, }) : _type = _CBuilderType.library; CBuilder.executable({ @@ -99,9 +114,11 @@ class CBuilder implements Builder { this.defines = const {}, this.buildModeDefine = true, this.ndebugDefine = true, + bool? pie = false, }) : _type = _CBuilderType.executable, assetId = null, - installName = null; + installName = null, + pic = pie; /// Runs the C Compiler with on this C build spec. /// @@ -148,6 +165,7 @@ class CBuilder implements Builder { if (ndebugDefine && buildConfig.buildMode != BuildMode.debug) 'NDEBUG': null, }, + pic: pic, ); await task.run(); } diff --git a/pkgs/native_toolchain_c/lib/src/cbuilder/run_cbuilder.dart b/pkgs/native_toolchain_c/lib/src/cbuilder/run_cbuilder.dart index f68750ee3..89d0f3eb3 100644 --- a/pkgs/native_toolchain_c/lib/src/cbuilder/run_cbuilder.dart +++ b/pkgs/native_toolchain_c/lib/src/cbuilder/run_cbuilder.dart @@ -25,7 +25,7 @@ class RunCBuilder { final Uri outDir; final Target target; - /// The install of the [dynamicLibrary]. + /// The install name of the [dynamicLibrary]. /// /// Can be inspected with `otool -D `. /// @@ -33,6 +33,7 @@ class RunCBuilder { final Uri? installName; final Map defines; + final bool? pic; RunCBuilder({ required this.buildConfig, @@ -43,6 +44,7 @@ class RunCBuilder { this.staticLibrary, this.installName, this.defines = const {}, + this.pic, }) : outDir = buildConfig.outDir, target = buildConfig.target, assert([executable, dynamicLibrary, staticLibrary] @@ -145,6 +147,20 @@ class RunCBuilder { '-o', outDir.resolve('out.o').toFilePath(), ], + if (pic != null) + if (pic!) ...[ + if (dynamicLibrary != null) '-fPIC', + // Using PIC for static libraries allows them to be linked into + // any executable, but it is not necessarily the best option in + // terms of overhead. We would have to know wether the target into + // which the static library is linked is PIC, PIE or neither. Then + // we could use the same option for the static library. + if (staticLibrary != null) '-fPIC', + if (executable != null) '-fPIE', + ] else ...[ + '-fno-PIC', + '-fno-PIE', + ], for (final MapEntry(key: name, :value) in defines.entries) if (value == null) '-D$name' else '-D$name=$value', ], diff --git a/pkgs/native_toolchain_c/pubspec.yaml b/pkgs/native_toolchain_c/pubspec.yaml index 6d4a63b74..6cc5c9de9 100644 --- a/pkgs/native_toolchain_c/pubspec.yaml +++ b/pkgs/native_toolchain_c/pubspec.yaml @@ -1,7 +1,7 @@ name: native_toolchain_c description: >- A library to invoke the native C compiler installed on the host machine. -version: 0.2.1 +version: 0.2.2 repository: https://github.com/dart-lang/native/tree/main/pkgs/native_toolchain_c topics: diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_android_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_android_test.dart index e12a4ee3e..276c618da 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_android_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_android_test.dart @@ -41,7 +41,7 @@ void main() { for (final linkMode in LinkMode.values) { for (final target in targets) { - test('Cbuilder $linkMode library $target', () async { + test('CBuilder $linkMode library $target', () async { await inTempDir((tempUri) async { final libUri = await buildLib( tempUri, @@ -77,7 +77,7 @@ void main() { } } - test('Cbuilder API levels binary difference', () async { + test('CBuilder API levels binary difference', () async { const target = Target.androidArm64; const linkMode = LinkMode.dynamic; const apiLevel1 = flutterAndroidNdkVersionLowestSupported; diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_ios_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_ios_test.dart index 5c467134c..ff013804f 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_ios_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_ios_test.dart @@ -50,7 +50,7 @@ void main() { Uri.file('@executable_path/Frameworks/$libName'), ]) { test( - 'Cbuilder $linkMode library $targetIOSSdk $target' + 'CBuilder $linkMode library $targetIOSSdk $target' ' ${installName ?? ''}' .trim(), () async { await inTempDir((tempUri) async { diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_linux_host_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_linux_host_test.dart index 51ca505ae..4ac497b12 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_linux_host_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_linux_host_test.dart @@ -36,7 +36,7 @@ void main() { for (final linkMode in LinkMode.values) { for (final target in targets) { - test('Cbuilder $linkMode library $target', () async { + test('CBuilder $linkMode library $target', () async { await inTempDir((tempUri) async { final addCUri = packageUri.resolve('test/cbuilder/testfiles/add/src/add.c'); diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_macos_host_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_macos_host_test.dart index 579bbdaf7..43f091eec 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_macos_host_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_macos_host_test.dart @@ -36,7 +36,7 @@ void main() { for (final linkMode in LinkMode.values) { for (final target in targets) { - test('Cbuilder $linkMode library $target', () async { + test('CBuilder $linkMode library $target', () async { await inTempDir((tempUri) async { final addCUri = packageUri.resolve('test/cbuilder/testfiles/add/src/add.c'); diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_windows_host_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_windows_host_test.dart index 68fa4a83b..4d322eb39 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_windows_host_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_cross_windows_host_test.dart @@ -48,7 +48,7 @@ void main() { for (final linkMode in LinkMode.values) { for (final target in targets) { - test('Cbuilder $linkMode library $target', () async { + test('CBuilder $linkMode library $target', () async { await inTempDir((tempUri) async { final addCUri = packageUri.resolve('test/cbuilder/testfiles/add/src/add.c'); diff --git a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_test.dart b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_test.dart index d1f63ff0f..a0592be7a 100644 --- a/pkgs/native_toolchain_c/test/cbuilder/cbuilder_test.dart +++ b/pkgs/native_toolchain_c/test/cbuilder/cbuilder_test.dart @@ -19,94 +19,44 @@ import 'package:test/test.dart'; import '../helpers.dart'; void main() { - for (final buildMode in BuildMode.values) { - test('Cbuilder executable $buildMode', () async { - await inTempDir((tempUri) async { - final helloWorldCUri = packageUri - .resolve('test/cbuilder/testfiles/hello_world/src/hello_world.c'); - if (!await File.fromUri(helloWorldCUri).exists()) { - throw Exception('Run the test from the root directory.'); - } - const name = 'hello_world'; - - final buildConfig = BuildConfig( - outDir: tempUri, - packageRoot: tempUri, - targetArchitecture: Architecture.current, - targetOs: OS.current, - buildMode: buildMode, - // Ignored by executables. - linkModePreference: LinkModePreference.dynamic, - cCompiler: CCompilerConfig( - cc: cc, - envScript: envScript, - envScriptArgs: envScriptArgs, - ), - ); - final buildOutput = BuildOutput(); - final cbuilder = CBuilder.executable( - name: name, - sources: [helloWorldCUri.toFilePath()], - ); - await cbuilder.run( - buildConfig: buildConfig, - buildOutput: buildOutput, - logger: logger, - ); + for (final pic in [null, true, false]) { + final picTag = + switch (pic) { null => 'auto_pic', true => 'pic', false => 'no_pic' }; - final executableUri = - tempUri.resolve(Target.current.os.executableFileName(name)); - expect(await File.fromUri(executableUri).exists(), true); - final result = await runProcess( - executable: executableUri, - logger: logger, - ); - expect(result.exitCode, 0); - if (buildMode == BuildMode.debug) { - expect(result.stdout.trim(), startsWith('Running in debug mode.')); - } - expect(result.stdout.trim(), endsWith('Hello world.')); - }); - }); - } + for (final buildMode in BuildMode.values) { + final suffix = testSuffix([buildMode, picTag]); - for (final dryRun in [true, false]) { - final testSuffix = dryRun ? ' dry_run' : ''; - test('Cbuilder dylib$testSuffix', () async { - await inTempDir( - // https://github.com/dart-lang/sdk/issues/40159 - keepTemp: Platform.isWindows, - (tempUri) async { - final addCUri = - packageUri.resolve('test/cbuilder/testfiles/add/src/add.c'); - const name = 'add'; - - final buildConfig = dryRun - ? BuildConfig.dryRun( - outDir: tempUri, - packageRoot: tempUri, - targetOs: OS.current, - linkModePreference: LinkModePreference.dynamic, - ) - : BuildConfig( - outDir: tempUri, - packageRoot: tempUri, - targetArchitecture: Architecture.current, - targetOs: OS.current, - buildMode: BuildMode.release, - linkModePreference: LinkModePreference.dynamic, - cCompiler: CCompilerConfig( - cc: cc, - envScript: envScript, - envScriptArgs: envScriptArgs, - ), - ); - final buildOutput = BuildOutput(); + test('CBuilder executable$suffix', () async { + await inTempDir((tempUri) async { + final helloWorldCUri = packageUri + .resolve('test/cbuilder/testfiles/hello_world/src/hello_world.c'); + if (!await File.fromUri(helloWorldCUri).exists()) { + throw Exception('Run the test from the root directory.'); + } + const name = 'hello_world'; + + final logMessages = []; + final logger = createCapturingLogger(logMessages); - final cbuilder = CBuilder.library( - sources: [addCUri.toFilePath()], + final buildConfig = BuildConfig( + outDir: tempUri, + packageRoot: tempUri, + targetArchitecture: Architecture.current, + targetOs: OS.current, + buildMode: buildMode, + // Ignored by executables. + linkModePreference: LinkModePreference.dynamic, + cCompiler: CCompilerConfig( + cc: cc, + envScript: envScript, + envScriptArgs: envScriptArgs, + ), + ); + final buildOutput = BuildOutput(); + final cbuilder = CBuilder.executable( name: name, - assetId: name, + sources: [helloWorldCUri.toFilePath()], + pie: pic, ); await cbuilder.run( buildConfig: buildConfig, @@ -114,25 +64,125 @@ void main() { logger: logger, ); - final dylibUri = - tempUri.resolve(Target.current.os.dylibFileName(name)); - expect(await File.fromUri(dylibUri).exists(), !dryRun); - if (!dryRun) { - final dylib = DynamicLibrary.open(dylibUri.toFilePath()); - final add = dylib.lookupFunction('add'); - expect(add(1, 2), 3); + final executableUri = + tempUri.resolve(Target.current.os.executableFileName(name)); + expect(await File.fromUri(executableUri).exists(), true); + final result = await runProcess( + executable: executableUri, + logger: logger, + ); + expect(result.exitCode, 0); + if (buildMode == BuildMode.debug) { + expect(result.stdout.trim(), startsWith('Running in debug mode.')); } - }, - ); - }); + expect(result.stdout.trim(), endsWith('Hello world.')); + + final compilerInvocation = logMessages.singleWhere( + (message) => message.contains(helloWorldCUri.toFilePath()), + ); + + switch ((buildConfig.targetOs, pic)) { + case (OS.windows, _) || (_, null): + expect(compilerInvocation, isNot(contains('-fPIC'))); + expect(compilerInvocation, isNot(contains('-fPIE'))); + expect(compilerInvocation, isNot(contains('-fno-PIC'))); + expect(compilerInvocation, isNot(contains('-fno-PIE'))); + case (_, true): + expect(compilerInvocation, contains('-fPIE')); + case (_, false): + expect(compilerInvocation, contains('-fno-PIC')); + expect(compilerInvocation, contains('-fno-PIE')); + } + }); + }); + } + + for (final dryRun in [true, false]) { + final suffix = testSuffix([if (dryRun) 'dry_run', picTag]); + + test('CBuilder dylib$suffix', () async { + await inTempDir( + // https://github.com/dart-lang/sdk/issues/40159 + keepTemp: Platform.isWindows, + (tempUri) async { + final addCUri = + packageUri.resolve('test/cbuilder/testfiles/add/src/add.c'); + const name = 'add'; + + final logMessages = []; + final logger = createCapturingLogger(logMessages); + + final buildConfig = dryRun + ? BuildConfig.dryRun( + outDir: tempUri, + packageRoot: tempUri, + targetOs: OS.current, + linkModePreference: LinkModePreference.dynamic, + ) + : BuildConfig( + outDir: tempUri, + packageRoot: tempUri, + targetArchitecture: Architecture.current, + targetOs: OS.current, + buildMode: BuildMode.release, + linkModePreference: LinkModePreference.dynamic, + cCompiler: CCompilerConfig( + cc: cc, + envScript: envScript, + envScriptArgs: envScriptArgs, + ), + ); + final buildOutput = BuildOutput(); + + final cbuilder = CBuilder.library( + sources: [addCUri.toFilePath()], + name: name, + assetId: name, + pic: pic, + ); + await cbuilder.run( + buildConfig: buildConfig, + buildOutput: buildOutput, + logger: logger, + ); + + final dylibUri = + tempUri.resolve(Target.current.os.dylibFileName(name)); + expect(await File.fromUri(dylibUri).exists(), !dryRun); + if (!dryRun) { + final dylib = DynamicLibrary.open(dylibUri.toFilePath()); + final add = dylib.lookupFunction('add'); + expect(add(1, 2), 3); + + final compilerInvocation = logMessages.singleWhere( + (message) => message.contains(addCUri.toFilePath()), + ); + switch ((buildConfig.targetOs, pic)) { + case (OS.windows, _) || (_, null): + expect(compilerInvocation, isNot(contains('-fPIC'))); + expect(compilerInvocation, isNot(contains('-fPIE'))); + expect(compilerInvocation, isNot(contains('-fno-PIC'))); + expect(compilerInvocation, isNot(contains('-fno-PIE'))); + case (_, true): + expect(compilerInvocation, contains('-fPIC')); + case (_, false): + expect(compilerInvocation, contains('-fno-PIC')); + expect(compilerInvocation, contains('-fno-PIE')); + } + } + }, + ); + }); + } } for (final buildMode in BuildMode.values) { for (final enabled in [true, false]) { + final suffix = testSuffix([buildMode, enabled ? 'enabled' : 'disabled']); + test( - 'Cbuilder build mode defines ${enabled ? 'enabled' : 'disabled'} for ' - '$buildMode', + 'CBuilder build mode defines$suffix', () => testDefines( buildMode: buildMode, buildModeDefine: enabled, @@ -143,8 +193,10 @@ void main() { } for (final value in [true, false]) { + final suffix = testSuffix([value ? 'with_value' : 'without_value']); + test( - 'Cbuilder define ${value ? 'with' : 'without'} value', + 'CBuilder define$suffix', () => testDefines(customDefineWithValue: value), ); } diff --git a/pkgs/native_toolchain_c/test/helpers.dart b/pkgs/native_toolchain_c/test/helpers.dart index f32fb1dea..c43bf93a1 100644 --- a/pkgs/native_toolchain_c/test/helpers.dart +++ b/pkgs/native_toolchain_c/test/helpers.dart @@ -11,6 +11,32 @@ import 'package:native_toolchain_c/src/native_toolchain/apple_clang.dart'; import 'package:native_toolchain_c/src/utils/run_process.dart'; import 'package:test/test.dart'; +/// Returns a suffix for a test that is parameterized. +/// +/// [tags] represent the current configuration of the test. Each element +/// is converted to a string by calling [Object.toString]. +/// +/// ## Example +/// +/// The instances of the test below will have the following descriptions: +/// +/// - `My test` +/// - `My test (dry_run)` +/// +/// ```dart +/// void main() { +/// for (final dryRun in [true, false]) { +/// final suffix = testSuffix([if (dryRun) 'dry_run']); +/// +/// test('My test$suffix', () {}); +/// } +/// } +/// ``` +String testSuffix(List tags) => switch (tags) { + [] => '', + _ => ' (${tags.join(', ')})', + }; + const keepTempKey = 'KEEP_TEMPORARY_DIRECTORIES'; Future inTempDir(