Skip to content

7903947: Access to function pointers in structs could be streamlined #287

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

7903947: Access to function pointers in structs could be streamlined #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/main/java/org/openjdk/jextract/JextractTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ private static List<JavaSourceFile> generateInternal(Declaration.Scoped decl,
return logger.hasErrors() ?
List.of() :
List.of(OutputFactory.generateWrapped(transformedDecl, targetPkg, options.libraries, options.useSystemLoadLibrary,
options.sharedClassName));
options.includeHelper, options.sharedClassName));
}

/**
Expand Down
58 changes: 42 additions & 16 deletions src/main/java/org/openjdk/jextract/impl/IncludeHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.Comparator;
import java.util.Collections;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -82,12 +84,35 @@ static IncludeKind fromScoped(Declaration.Scoped scoped) {
}

private final EnumMap<IncludeKind, Set<String>> includesSymbolNamesByKind = new EnumMap<>(IncludeKind.class);

private final Map<String, Set<String>> propertiesByName = new HashMap<>();

private final Set<Declaration> usedDeclarations = new HashSet<>();
public String dumpIncludesFile;

/**
* Register an include, may be:Name or Name,flag1,flag2 orName,prop=value,flag
*/
public void addSymbol(IncludeKind kind, String symbolName) {
Set<String> names = includesSymbolNamesByKind.computeIfAbsent(kind, (_unused) -> new HashSet<>());
names.add(symbolName);
String[] parts = symbolName.split(",", 2);
String name = parts[0].trim();

includesSymbolNamesByKind.computeIfAbsent(kind, k -> new HashSet<>())
.add(name);

if (parts.length == 2) {
Set<String> props = propertiesByName
.computeIfAbsent(name, k -> new HashSet<>());
for (String tok : parts[1].split(",")) {
props.add(tok.trim());
}
}

if (kind == IncludeKind.STRUCT || kind == IncludeKind.UNION) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I get this logic -- it seems like we're always setting a property P for a typedef with name N whenever we see it set for a struct N? That seems odd.

But in example as the one you added in the test are a bit ambiguous, as the same declaration induces both an anonymous struct decl and a typedef, so it is less clear which is which. If I write:

typedef struct Foo {
int a;
};

It seems to be that jextract only retains a "struct" tree, and throws away the typedef. In fact I was surprised that your test worked at all, as in the above case dump-includes would include something like this:

--include-struct Foo   # header: ....

And not:

--include-typedef Foo   # header: ....

So adding a property on the typedef should have no effect?

includesSymbolNamesByKind
.computeIfAbsent(IncludeKind.TYPEDEF, k -> new HashSet<>())
.add(name);
}
}

public boolean isIncluded(Declaration.Variable variable) {
Expand All @@ -111,24 +136,24 @@ public boolean isIncluded(Declaration.Scoped scoped) {
}

private boolean checkIncludedAndAddIfNeeded(IncludeKind kind, Declaration declaration) {
boolean included = isIncludedInternal(kind, declaration);
boolean included = !isEnabled()
|| includesSymbolNamesByKind
.getOrDefault(kind, Collections.emptySet())
.contains(declaration.name());
if (included && dumpIncludesFile != null) {
usedDeclarations.add(declaration);
}
return included;
}

private boolean isIncludedInternal(IncludeKind kind, Declaration declaration) {
if (!isEnabled()) {
return true;
} else {
Set<String> names = includesSymbolNamesByKind.computeIfAbsent(kind, (_unused) -> new HashSet<>());
return names.contains(declaration.name());
}
public boolean isEnabled() {
return !includesSymbolNamesByKind.isEmpty();
}

public boolean isEnabled() {
return includesSymbolNamesByKind.size() > 0;
public boolean isFunctionalDispatch(String name) {
return propertiesByName
.getOrDefault(name, Collections.emptySet())
.contains("functional");
}

public void dumpIncludes() {
Expand All @@ -139,8 +164,9 @@ public void dumpIncludes() {
Collectors.toCollection(() -> new TreeSet<>(Comparator.comparing(Declaration::name)))));
String lineSep = "";
for (Map.Entry<Path, Set<Declaration>> pathEntries : declsByPath.entrySet()) {
writer.append(lineSep);
writer.append("#### Extracted from: " + pathEntries.getKey().toString() + "\n\n");
writer.append(lineSep)
.append("#### Extracted from: ")
.append(pathEntries.getKey().toString()).append("\n\n");
Map<IncludeKind, List<Declaration>> declsByKind = pathEntries.getValue().stream()
.collect(Collectors.groupingBy(IncludeKind::fromDeclaration));
int maxLengthOptionCol = pathEntries.getValue().stream().mapToInt(d -> d.name().length()).max().getAsInt();
Expand All @@ -149,9 +175,9 @@ public void dumpIncludes() {
maxLengthOptionCol += 1; // space
for (Map.Entry<IncludeKind, List<Declaration>> kindEntries : declsByKind.entrySet()) {
for (Declaration d : kindEntries.getValue()) {
writer.append(String.format("%-" + maxLengthOptionCol + "s %s",
writer.append(String.format("%-" + maxLengthOptionCol + "s %s\n",
"--" + kindEntries.getKey().optionName() + " " + d.name(),
"# header: " + pathEntries.getKey() + "\n"));
"# header: " + pathEntries.getKey()));
}
}
lineSep = "\n";
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/openjdk/jextract/impl/OutputFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,11 @@ public static JavaSourceFile[] generateWrapped(Declaration.Scoped decl,
String pkgName,
List<Options.Library> libs,
boolean useSystemLoadLibrary,
IncludeHelper includeHelper,
String sharedClassName) {
String clsName = JavaName.getOrThrow(decl);
ToplevelBuilder toplevelBuilder = new ToplevelBuilder(pkgName, clsName,
libs, useSystemLoadLibrary, sharedClassName);
libs, useSystemLoadLibrary, sharedClassName, includeHelper);
return new OutputFactory(toplevelBuilder).generate(decl);
}

Expand Down
100 changes: 91 additions & 9 deletions src/main/java/org/openjdk/jextract/impl/StructBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,18 @@ final class StructBuilder extends ClassSourceBuilder implements OutputFactory.Bu
private final Declaration.Scoped structTree;
private final Type structType;
private final Deque<Declaration> nestedAnonDeclarations;
private final IncludeHelper includeHelper;
private final boolean functionalDispatch;

StructBuilder(SourceFileBuilder builder, String modifiers, String className,
ClassSourceBuilder enclosing, String runtimeHelperName, Declaration.Scoped structTree) {
ClassSourceBuilder enclosing, String runtimeHelperName, Declaration.Scoped structTree,
IncludeHelper includeHelper) {
super(builder, modifiers, Kind.CLASS, className, null, enclosing, runtimeHelperName);
this.structTree = structTree;
this.structType = Type.declared(structTree);
this.nestedAnonDeclarations = new ArrayDeque<>();
this.functionalDispatch = includeHelper.isFunctionalDispatch(builder.className());
this.includeHelper = includeHelper;
}

private String safeParameterName(String paramName) {
Expand Down Expand Up @@ -116,7 +121,7 @@ public StructBuilder addStruct(Declaration.Scoped tree) {
return this;
} else {
StructBuilder builder = new StructBuilder(sourceFileBuilder(), "public static",
JavaName.getOrThrow(tree), this, runtimeHelperName(), tree);
JavaName.getOrThrow(tree), this, runtimeHelperName(), tree, this.includeHelper);
builder.begin();
return builder;
}
Expand All @@ -137,25 +142,102 @@ public void addVar(Declaration.Variable varTree) {
String layoutField = emitLayoutFieldDecl(varTree, javaName);
appendBlankLine();
String offsetField = emitOffsetFieldDecl(varTree, javaName);
if (Utils.isArray(varTree.type()) || Utils.isStructOrUnion(varTree.type())) {
Type type = varTree.type();
if (Utils.isArray(type) || Utils.isStructOrUnion(type)) {
emitSegmentGetter(javaName, varTree, offsetField, layoutField);
emitSegmentSetter(javaName, varTree, offsetField, layoutField);
int dims = Utils.dimensions(varTree.type()).size();
int dims = Utils.dimensions(type).size();
if (dims > 0) {
emitDimensionsFieldDecl(varTree, javaName);
String arrayHandle = emitArrayElementHandle(javaName, varTree, layoutField, dims);
IndexList indexList = IndexList.of(dims);
emitFieldArrayGetter(javaName, varTree, arrayHandle, indexList);
emitFieldArraySetter(javaName, varTree, arrayHandle, indexList);
}
} else if (Utils.isPointer(varTree.type()) || Utils.isPrimitive(varTree.type())) {
emitFieldGetter(javaName, varTree, layoutField, offsetField);
emitFieldSetter(javaName, varTree, layoutField, offsetField);
} else if (Utils.isPointer(type) || Utils.isPrimitive(type)) {
boolean isFuncPtr = functionalDispatch && Utils.isFunctionPointer(type);
if (isFuncPtr) {
emitFieldSetter(javaName, varTree, layoutField, offsetField);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we even want the setter -- the fields in these structs are typically populated by a library

emitFunctionalConvenience(
javaName,
(Type.Function)((Type.Delegated) type).type(),
varTree,
layoutField,
offsetField
);
} else {
emitFieldGetter(javaName, varTree, layoutField, offsetField);
emitFieldSetter(javaName, varTree, layoutField, offsetField);
}
} else {
throw new IllegalArgumentException(String.format("Type not supported: %1$s", varTree.type()));
throw new IllegalArgumentException("Type not supported: " + type);
}
}

/**
* Generates exactly one invoker, inlining the struct.get(...) to avoid name collision.
*/
private void emitFunctionalConvenience(String invokerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this emitInvoker

Type.Function funcType,
Declaration.Variable varTree,
String layoutField,
String offsetField) {
String returnType = Utils.carrierFor(funcType.returnType()).getSimpleName();
List<Type> params = funcType.argumentTypes();

String sig = IntStream.range(0, params.size())
.mapToObj(i -> Utils.carrierFor(params.get(i)).getSimpleName() + " _x" + i)
.collect(Collectors.joining(", "));
String args = IntStream.range(0, params.size())
.mapToObj(i -> "_x" + i)
.collect(Collectors.joining(", "));

boolean structRet = !Utils.isPrimitive(funcType.returnType())
&& !Utils.isPointer(funcType.returnType());
String retKw = returnType.equals("void") ? "" : "return ";

appendBlankLine();
incrAlign();
emitDocComment(varTree, "Invoker for the pointer inside the Struct:");
decrAlign();

if (structRet) {
appendIndentedLines(
"public static %1$s %2$s(MemorySegment struct, SegmentAllocator alloc%3$s) {",
returnType, invokerName, sig.isEmpty() ? "" : ", " + sig
);
incrAlign();
appendIndentedLines(
"MemorySegment fp = struct.get(%1$s, %2$s);",
layoutField, offsetField
);
appendIndentedLines(
"%1$s%2$s.invoke(fp, alloc%3$s);",
retKw,
JavaFunctionalInterfaceName.getOrThrow(varTree),
args.isEmpty() ? "" : ", " + args
);
} else {
appendIndentedLines(
"public static %1$s %2$s(MemorySegment struct%3$s) {",
returnType, invokerName, sig.isEmpty() ? "" : ", " + sig
);
incrAlign();
appendIndentedLines(
"MemorySegment fp = struct.get(%1$s, %2$s);",
layoutField, offsetField
);
appendIndentedLines(
"%1$s%2$s.invoke(fp%3$s);",
retKw,
JavaFunctionalInterfaceName.getOrThrow(varTree),
args.isEmpty() ? "" : ", " + args
);
}
decrAlign();
appendIndentedLines("}");
}

private List<String> prefixNamesList() {
return nestedAnonDeclarations.stream()
.map(d -> AnonymousStruct.anonName((Declaration.Scoped)d))
Expand Down Expand Up @@ -490,7 +572,7 @@ private String structOrUnionLayoutString(long base, Declaration.Scoped scoped, i
offset += fieldSize;
size += fieldSize;
} else {
size = Math.max(size, ClangSizeOf.getOrThrow(member));
size = Math.max(size, fieldSize);
}
}
}
Expand Down
15 changes: 12 additions & 3 deletions src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@ class ToplevelBuilder implements OutputFactory.Builder {
private final List<SourceFileBuilder> otherBuilders = new ArrayList<>();
private HeaderFileBuilder lastHeader;
private final ClassDesc headerDesc;
private final IncludeHelper includeHelper;

ToplevelBuilder(String packageName, String headerClassName, List<Options.Library> libs,
boolean useSystemLoadLibrary, String sharedClassName) {
boolean useSystemLoadLibrary, String sharedClassName, IncludeHelper includeHelper) {
this.headerDesc = ClassDesc.of(packageName, headerClassName);
this.includeHelper = includeHelper;
shared = sharedClassName != null ?
sharedClassName :
headerDesc.displayName() + "$shared";
Expand Down Expand Up @@ -126,14 +128,14 @@ public List<JavaSourceFile> toFiles() {
// adjust suffixes so that the last header class becomes the main header class,
// and extends all the other header classes
int totalHeaders = headerBuilders.size();
String className = headerBuilders.get(0).className();
for (int i = 0; i < totalHeaders; i++) {
SourceFileBuilder header = headerBuilders.get(i);
boolean isMainHeader = (i == totalHeaders - 1); // last header is the main header
String currentSuffix = isMainHeader ?
"" : // main header class, drop the suffix
String.format("_%d", totalHeaders - i - 1);
String preSuffix = String.format("_%d", totalHeaders - i);
String className = headerBuilders.getFirst().className();
String modifier = isMainHeader ? "public " : "";

files.add(header.toFile(currentSuffix, s ->
Expand Down Expand Up @@ -192,7 +194,14 @@ public void addTypedef(Declaration.Typedef typedefTree, String superClass, Type
public StructBuilder addStruct(Declaration.Scoped tree) {
SourceFileBuilder sfb = SourceFileBuilder.newSourceFile(packageName(), JavaName.getOrThrow(tree));
otherBuilders.add(sfb);
StructBuilder structBuilder = new StructBuilder(sfb, "public", sfb.className(), null, mainHeaderClassName(), tree);
StructBuilder structBuilder = new StructBuilder(sfb,
"public",
sfb.className(),
null,
mainHeaderClassName(),
tree,
includeHelper
);
structBuilder.begin();
return structBuilder;
}
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/org/openjdk/jextract/impl/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -151,6 +151,17 @@ static boolean isPointer(Type type) {
};
}

static boolean isFunctionPointer(Type type) {
return switch (type) {
case Type.Delegated delegated when delegated.kind() == Delegated.Kind.POINTER ->
delegated.type() instanceof Type.Function;
case Type.Delegated delegated when delegated.kind() == Delegated.Kind.TYPEDEF ->
isFunctionPointer(delegated.type());
default -> false;
};
}


static boolean isPrimitive(Type type) {
return switch (type) {
case Type.Declared declared when declared.tree().kind() == Declaration.Scoped.Kind.ENUM -> true;
Expand Down
Loading