Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ffigen] Add config ignore-source-errors #810

Merged
merged 19 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkgs/ffigen/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 11.0.0-wip

- Any compiler errors/warnings in source header files will now result in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of warnings result in wrong generated code? Should we refuse to generate on all warnings?
If we refuse to generate for too many warnings, people might just start using ignore-source-errors instead of fixing the issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, for now I've only handled warnings/errors from libclang. (Excluding ObjC for now)

But I think anything which can potentially generate invalid bindings, that might silently break at runtime should be covered.

bindings to **not** be generated by default, since it may result in invalid
bindings if the compiler makes a wrong guess. A flag `--ignore-source-errors` (or yaml config `ignore-source-errors: true`)
must be passed to change this behaviour.

## 10.0.0

- Stable release targeting Dart 3.2 using new `dart:ffi` features available
Expand Down
18 changes: 18 additions & 0 deletions pkgs/ffigen/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,24 @@ use-supported-typedefs: true
```yaml
use-dart-handle: true
```

</td>
</tr>
<tr>
<td>ignore-source-errors</td>
<td>Where to ignore compiler warnings/errors in source header files.<br>
<b>Default: false</b>
</td>
<td>

```yaml
ignore-source-errors: true
```
and/or via the command line -
```bash
dart run ffigen --ignore-source-errors
```

</td>
</tr>
<tr>
Expand Down
26 changes: 26 additions & 0 deletions pkgs/ffigen/doc/errors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Errors in ffigen

This file documents various errors and their potential fixes related to ffigen.

## Errors in source header files

FFIgen uses libclang to parse header files. Any compiler warnings/errors should be logged (with SEVERE level).
Compiler errors and warnings should be resolved as they can potentially generate invalid bindings that might cause silent errors and crashes at runtime.

> You can pass in args to libclang using `compiler-opts` via cmd line or yaml config or both.
Here we'll list some common usecases. You can find the full list of [supported args here](https://clang.llvm.org/docs/ClangCommandLineReference.html#id5).

### Missing headers

These are the most common source file errors. You can specify [include paths to clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#id6) like this
```yaml
compiler-opts:
- "-I/path/to/folder"
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/?

@liamappelbe what are typical errors we see with Objective-C?

### Ignoring source errors
As a last resort, you can pass in `--ignore-source-errors` or set `ignore-source-errors: true` in yaml config.
mannprerak2 marked this conversation as resolved.
Show resolved Hide resolved

**Warning: This will likely lead to incorrect bindings!**
5 changes: 4 additions & 1 deletion pkgs/ffigen/ffigen.schema.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"$id": "https://json.schemastore.org/ffigen",
"$comment": "This file is generated. To regenerate run: dart tool/generate_json_schema.dart in github.com/dart-lang/ffigen",
"$comment": "This file is generated. To regenerate run: dart tool/generate_json_schema.dart in github.com/dart-lang/native/tree/main/pkgs/ffigen",
"$schema": "https://json-schema.org/draft/2020-12/schema",
"type": "object",
"additionalProperties": false,
Expand Down Expand Up @@ -73,6 +73,9 @@
"entry-points"
]
},
"ignore-source-errors": {
"type": "boolean"
},
"compiler-opts": {
"$oneOf": [
{
Expand Down
12 changes: 12 additions & 0 deletions pkgs/ffigen/lib/src/config_provider/config.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ class Config {
FfiNativeConfig get ffiNativeConfig => _ffiNativeConfig;
late FfiNativeConfig _ffiNativeConfig;

/// Where to ignore compiler warnings/errors in source header files.
bool ignoreSourceErrors = false;

Config._({required this.filename, required this.packageConfig});

/// Create config from Yaml map.
Expand Down Expand Up @@ -285,6 +288,15 @@ class Config {
transform: (node) => headersExtractor(node.value, filename),
result: (node) => _headers = node.value,
)),
HeterogeneousMapEntry(
key: strings.ignoreSourceErrors,
valueConfigSpec: BoolConfigSpec(),
defaultValue: (node) => false,
resultOrDefault: (node) {
// Set value to true if not already.
ignoreSourceErrors = ignoreSourceErrors || node.value as bool;
},
),
HeterogeneousMapEntry(
key: strings.compilerOpts,
valueConfigSpec: OneOfConfigSpec<List<String>, List<String>>(
Expand Down
2 changes: 1 addition & 1 deletion pkgs/ffigen/lib/src/config_provider/config_spec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ abstract class ConfigSpec<TE extends Object?, RE extends Object?> {
return {
r"$id": schemaId,
r"$comment":
"This file is generated. To regenerate run: dart tool/generate_json_schema.dart in github.com/dart-lang/ffigen",
"This file is generated. To regenerate run: dart tool/generate_json_schema.dart in github.com/dart-lang/native/tree/main/pkgs/ffigen",
r"$schema": "https://json-schema.org/draft/2020-12/schema",
...schemaMap,
r"$defs": defs,
Expand Down
10 changes: 10 additions & 0 deletions pkgs/ffigen/lib/src/executables/ffigen.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ final _logger = Logger('ffigen.ffigen');
final _ansi = Ansi(Ansi.terminalSupportsAnsi);

const compilerOpts = 'compiler-opts';
const ignoreSourceErrors = 'ignore-source-errors';
const conf = 'config';
const help = 'help';
const verbose = 'verbose';
Expand Down Expand Up @@ -87,6 +88,10 @@ Config getConfig(ArgResults result, PackageConfig? packageConfig) {
highPriority: true);
}

if (result.wasParsed(ignoreSourceErrors)) {
config.ignoreSourceErrors = true;
}

return config;
}

Expand Down Expand Up @@ -158,6 +163,11 @@ ArgResults getArgResults(List<String> args) {
compilerOpts,
help: 'Compiler options for clang. (E.g --$compilerOpts "-I/headers -W")',
);
parser.addFlag(
ignoreSourceErrors,
help: 'Ignore any compiler warnings/errors in source header files',
negatable: false,
);

ArgResults results;
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,21 @@ class Clang {
late final _clang_formatDiagnostic = _clang_formatDiagnosticPtr
.asFunction<CXString Function(CXDiagnostic, int)>();

/// Determine the severity of the given diagnostic.
int clang_getDiagnosticSeverity(
CXDiagnostic arg0,
) {
return _clang_getDiagnosticSeverity(
arg0,
);
}

late final _clang_getDiagnosticSeverityPtr =
_lookup<ffi.NativeFunction<ffi.Int32 Function(CXDiagnostic)>>(
'clang_getDiagnosticSeverity');
late final _clang_getDiagnosticSeverity =
_clang_getDiagnosticSeverityPtr.asFunction<int Function(CXDiagnostic)>();

/// Same as \c clang_parseTranslationUnit2, but returns
/// the \c CXTranslationUnit instead of an error code. In case of an error this
/// routine returns a \c NULL \c CXTranslationUnit, without further detailed
Expand Down Expand Up @@ -1484,6 +1499,29 @@ abstract class CXDiagnosticDisplayOptions {
static const int CXDiagnostic_DisplayCategoryName = 32;
}

/// Describes the severity of a particular diagnostic.
abstract class CXDiagnosticSeverity {
/// A diagnostic that has been suppressed, e.g., by a command-line
/// option.
static const int CXDiagnostic_Ignored = 0;

/// This diagnostic is a note that should be attached to the
/// previous (non-note) diagnostic.
static const int CXDiagnostic_Note = 1;

/// This diagnostic indicates suspicious code that may not be
/// wrong.
static const int CXDiagnostic_Warning = 2;

/// This diagnostic indicates that the code is ill-formed.
static const int CXDiagnostic_Error = 3;

/// This diagnostic indicates that the code is ill-formed such
/// that future parser recovery is unlikely to produce useful
/// results.
static const int CXDiagnostic_Fatal = 4;
}

/// Flags that control the creation of translation units.
///
/// The enumerators in this enumeration type are meant to be bitwise
Expand Down Expand Up @@ -2641,8 +2679,10 @@ abstract class CXChildVisitResult {
/// The visitor should return one of the \c CXChildVisitResult values
/// to direct clang_visitCursorChildren().
typedef CXCursorVisitor
= ffi.Pointer<ffi.NativeFunction<CXCursorVisitor_function>>;
typedef CXCursorVisitor_function = ffi.Int32 Function(
= ffi.Pointer<ffi.NativeFunction<CXCursorVisitorFunction>>;
typedef CXCursorVisitorFunction = ffi.Int32 Function(
CXCursor cursor, CXCursor parent, CXClientData client_data);
typedef DartCXCursorVisitorFunction = int Function(
CXCursor cursor, CXCursor parent, CXClientData client_data);

/// Opaque pointer representing client data that will be passed through
Expand Down
5 changes: 5 additions & 0 deletions pkgs/ffigen/lib/src/header_parser/data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ List<Constant> _unnamedEnumConstants = [];
ObjCBuiltInFunctions get objCBuiltInFunctions => _objCBuiltInFunctions;
late ObjCBuiltInFunctions _objCBuiltInFunctions;

/// Tracks if any source error/warning has occured which can potentially cause
/// invalid generated bindings.
bool hasSourceErrors = false;

void initializeGlobals({required Config config}) {
_config = config;
_clang = Clang(DynamicLibrary.open(config.libclangDylib));
Expand All @@ -54,4 +58,5 @@ void initializeGlobals({required Config config}) {
_cursorIndex = CursorIndex();
_bindingsIndex = BindingsIndex();
_objCBuiltInFunctions = ObjCBuiltInFunctions();
hasSourceErrors = false;
}
19 changes: 17 additions & 2 deletions pkgs/ffigen/lib/src/header_parser/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import 'utils.dart';
Library parse(Config c) {
initParser(c);

final bindings = parseToBindings();
final bindings = parseToBindings(c);

final library = Library(
bindings: bindings,
Expand Down Expand Up @@ -52,7 +52,7 @@ void initParser(Config c) {
}

/// Parses source files and adds generated bindings to [bindings].
List<Binding> parseToBindings() {
List<Binding> parseToBindings(Config c) {
final index = clang.clang_createIndex(0, 0);

Pointer<Pointer<Utf8>> clangCmdArgs = nullptr;
Expand Down Expand Up @@ -113,6 +113,21 @@ List<Binding> parseToBindings() {
tuList.add(tu);
}

if (hasSourceErrors) {
_logger.warning("The compiler found warnings/errors in source files.");
_logger.warning("This will likely generate invalid bindings.");
if (config.ignoreSourceErrors) {
_logger.warning(
"Ignored source errors. (User supplied --ignore-source-errors)");
} else if (config.language == Language.objc) {
_logger.warning("Ignored source errors. (ObjC)");
} else {
_logger.severe(
"Skipped generating bindings due to errors in source files. See https://github.com/dart-lang/native/blob/main/pkgs/ffigen/doc/errors.md.");
exit(1);
}
}

final tuCursors =
tuList.map((tu) => clang.clang_getTranslationUnitCursor(tu));

Expand Down
4 changes: 4 additions & 0 deletions pkgs/ffigen/lib/src/header_parser/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ void logTuDiagnostics(
logger.log(logLevel, 'Header $header: Total errors/warnings: $total.');
for (var i = 0; i < total; i++) {
final diag = clang.clang_getDiagnostic(tu, i);
if (clang.clang_getDiagnosticSeverity(diag) >=
clang_types.CXDiagnosticSeverity.CXDiagnostic_Warning) {
hasSourceErrors = true;
}
final cxstring = clang.clang_formatDiagnostic(
diag,
clang_types
Expand Down
1 change: 1 addition & 0 deletions pkgs/ffigen/lib/src/strings.dart
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ const supportedNativeType_mappings = <String, SupportedNativeType>{
const sort = 'sort';
const useSupportedTypedefs = 'use-supported-typedefs';
const useDartHandle = 'use-dart-handle';
const ignoreSourceErrors = 'ignore-source-errors';

const comments = 'comments';
// Sub-fields of comments.
Expand Down
2 changes: 1 addition & 1 deletion pkgs/ffigen/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# BSD-style license that can be found in the LICENSE file.

name: ffigen
version: 10.0.0
version: 11.0.0-wip
description: >
Generator for FFI bindings, using LibClang to parse C, Objective-C, and Swift
files.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ ${strings.output}: 'unused'
${strings.headers}:
${strings.entryPoints}:
- 'test/header_parser_tests/forward_decl.h'
${strings.ignoreSourceErrors}: true
'''),
);
});
Expand Down
1 change: 1 addition & 0 deletions pkgs/ffigen/test/header_parser_tests/unions_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ ${strings.output}: 'unused'
${strings.headers}:
${strings.entryPoints}:
- 'test/header_parser_tests/unions.h'
${strings.ignoreSourceErrors}: true
'''),
);
});
Expand Down
1 change: 1 addition & 0 deletions pkgs/ffigen/tool/libclang_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ functions:
- clang_disposeIndex
- clang_getNumDiagnostics
- clang_getDiagnostic
- clang_getDiagnosticSeverity
mannprerak2 marked this conversation as resolved.
Show resolved Hide resolved
- clang_disposeDiagnostic
- clang_parseTranslationUnit
- clang_disposeTranslationUnit
Expand Down
Loading