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

Filter functions, extern vars, defines and unused types from included headers #113

Merged
merged 8 commits into from
Jul 10, 2018
9 changes: 7 additions & 2 deletions bindgen/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ add_executable(bindgen
defines/DefineFinderActionFactory.h
TypeTranslator.h
TypeTranslator.cpp
HeaderManager.h
HeaderManager.cpp
CycleDetection.h
Utils.h
ir/IR.h
Expand Down Expand Up @@ -72,6 +70,13 @@ add_executable(bindgen
ir/types/FunctionPointerType.h
ir/types/ArrayType.cpp
ir/types/ArrayType.h
ir/location/Location.h
ir/location/ScalaLocation.cpp
ir/location/ScalaLocation.h
ir/location/SourceLocation.cpp
ir/location/SourceLocation.h
ir/location/LocationManager.cpp
ir/location/LocationManager.h
)

if (STATIC_LINKING)
Expand Down
32 changes: 0 additions & 32 deletions bindgen/HeaderManager.cpp

This file was deleted.

15 changes: 0 additions & 15 deletions bindgen/HeaderManager.h

This file was deleted.

39 changes: 9 additions & 30 deletions bindgen/Main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,7 @@ int main(int argc, const char *argv[]) {

llvm::cl::opt<std::string> LibName("name", llvm::cl::cat(Category),
llvm::cl::desc("Library name"));
llvm::cl::opt<std::string> StdHeaders(
"std-headers", llvm::cl::cat(Category),
llvm::cl::desc("Path to a file with the list of headers for which "
"bindings\n"
"will not be generated. "
"The list contains header files names\n"
"and package names that contain bindings for these "
"headers.\nExample:\n"
"math.h=scala.scalanative.native.math\n"
"stdlib.h=scala.scalanative.native.stdlib"));
llvm::cl::opt<bool> PrintHeadersLocation(
"location", llvm::cl::cat(Category),
llvm::cl::desc("Print list of parsed headers"));

llvm::cl::opt<std::string> ExcludePrefix(
"exclude-prefix", llvm::cl::cat(Category),
llvm::cl::desc("Functions and unused typedefs will be removed if their "
Expand Down Expand Up @@ -64,13 +52,6 @@ int main(int argc, const char *argv[]) {
objectName = "nativeLib";
}

auto stdhead = StdHeaders.getValue();
if (!stdhead.empty()) {
headerMan.LoadConfig(stdhead);
}

locations.clear();

IR ir(libName, linkName, objectName, Package.getValue());

DefineFinderActionFactory defineFinderActionFactory(ir);
Expand All @@ -79,18 +60,16 @@ int main(int argc, const char *argv[]) {
return result;
}

ScalaFrontendActionFactory actionFactory(ir);
assert(op.getSourcePathList().size() == 1);

char *resolved = realpath(op.getSourcePathList()[0].c_str(), nullptr);
LocationManager locationManager(resolved);

ScalaFrontendActionFactory actionFactory(ir, locationManager);
result = Tool.run(&actionFactory);

auto printLoc = PrintHeadersLocation.getValue();
if (printLoc) {
for (const auto &location : locations) {
llvm::outs() << location.c_str();
}
} else {
ir.generate(ExcludePrefix.getValue());
llvm::outs() << ir;
}
ir.generate(ExcludePrefix.getValue());
llvm::outs() << ir;
llvm::outs().flush();
return result;
}
2 changes: 1 addition & 1 deletion bindgen/TypeTranslator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ TypeTranslator::translateStructOrUnionOrEnum(const clang::QualType &qtpe) {
/* type is not yet defined.
* TypeDef with nullptr will be created.
* nullptr will be replaced by actual type when the type is declared. */
typeDef = ir.addTypeDef(nameWithoutSpace, nullptr);
typeDef = ir.addTypeDef(nameWithoutSpace, nullptr, nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

Eventually passing a location here would allow us to provide a better diagnostic if an opaque type is used in a problematic way.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a good idea but we should not use location field for this.
Maybe create an optional field typeDeclarationLocation

Copy link
Member Author

Choose a reason for hiding this comment

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

I started doing it but it required passing Location instance to methods of TypeTranslator and it does not look good to me, so I decided to leave it for now.
I hope that after fixing #106 we will not see the error message often (except the case when one wants to generated bindings for broken header).

return typeDef;
}

Expand Down
1 change: 1 addition & 0 deletions bindgen/TypeTranslator.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "ir/IR.h"
#include "ir/location/LocationManager.h"
#include <clang/Tooling/Tooling.h>

class TypeTranslator {
Expand Down
7 changes: 7 additions & 0 deletions bindgen/defines/DefineFinder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ DefineFinder::DefineFinder(IR &ir, const clang::CompilerInstance &compiler,
void DefineFinder::MacroDefined(const clang::Token &macroNameTok,
const clang::MacroDirective *md) {
clang::SourceManager &sm = compiler.getSourceManager();
if (!sm.isInMainFile(md->getLocation())) {
/* include defines only from the original header */
return;
}
if (sm.isWrittenInMainFile(macroNameTok.getLocation()) && md->isDefined() &&
!md->getMacroInfo()->isFunctionLike()) {
/* save defines only from the given header.
Expand Down Expand Up @@ -100,6 +104,9 @@ void DefineFinder::MacroUndefined(const clang::Token &macroNameTok,
const clang::MacroDefinition &md,
const clang::MacroDirective *undef) {
clang::SourceManager &sm = compiler.getSourceManager();
if (!sm.isInMainFile(undef->getLocation())) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Having a test for this would be very valuable.

if (sm.isWrittenInMainFile(macroNameTok.getLocation()) &&
md.getMacroInfo() && !md.getMacroInfo()->isFunctionLike()) {
std::string macroName = macroNameTok.getIdentifierInfo()->getName();
Expand Down
6 changes: 4 additions & 2 deletions bindgen/ir/Enum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ Enum::Enum(std::string name, std::string type,

bool Enum::isAnonymous() const { return name.empty(); }

std::shared_ptr<TypeDef> Enum::generateTypeDef() {
std::shared_ptr<TypeDef>
Enum::generateTypeDef(std::shared_ptr<Location> location) {
assert(!isAnonymous());
return std::make_shared<TypeDef>("enum_" + name, shared_from_this());
return std::make_shared<TypeDef>("enum_" + name, shared_from_this(),
std::move(location));
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why the enum doesn't have a location itself, which can be "reused" here.

Also, from TypeDef::location doc string:

nullptr if type is located in main file or is generated.

this is generated, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, docstring is wrong, location is null only if it is generated

}

llvm::raw_ostream &operator<<(llvm::raw_ostream &s, const Enum &e) {
Expand Down
3 changes: 2 additions & 1 deletion bindgen/ir/Enum.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ class Enum : public PrimitiveType, public std::enable_shared_from_this<Enum> {

bool isAnonymous() const;

std::shared_ptr<TypeDef> generateTypeDef();
std::shared_ptr<TypeDef>
generateTypeDef(std::shared_ptr<Location> location);

std::string getName() const;

Expand Down
76 changes: 66 additions & 10 deletions bindgen/ir/IR.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "IR.h"
#include "../Utils.h"
#include "location/SourceLocation.h"

IR::IR(std::string libName, std::string linkName, std::string objectName,
std::string packageName)
Expand All @@ -13,48 +14,53 @@ void IR::addFunction(std::string name, std::vector<Parameter *> parameters,
}

std::shared_ptr<TypeDef> IR::addTypeDef(std::string name,
std::shared_ptr<Type> type) {
typeDefs.push_back(std::make_shared<TypeDef>(std::move(name), type));
std::shared_ptr<Type> type,
std::shared_ptr<Location> location) {
typeDefs.push_back(
std::make_shared<TypeDef>(std::move(name), type, std::move(location)));
return typeDefs.back();
}

std::shared_ptr<Type> IR::addEnum(std::string name, const std::string &type,
std::vector<Enumerator> enumerators) {
std::vector<Enumerator> enumerators,
std::shared_ptr<Location> location) {
std::shared_ptr<Enum> e =
std::make_shared<Enum>(std::move(name), type, std::move(enumerators));
enums.push_back(e);
if (!e->isAnonymous()) {
typeDefs.push_back(e->generateTypeDef());
typeDefs.push_back(e->generateTypeDef(std::move(location)));
return typeDefs.back();
}
return nullptr;
}

void IR::addStruct(std::string name, std::vector<Field *> fields,
uint64_t typeSize) {
uint64_t typeSize, std::shared_ptr<Location> location) {
std::shared_ptr<Struct> s =
std::make_shared<Struct>(name, std::move(fields), typeSize);
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'm curious why the location is associated with the generated typedef instead of the "concrete" type.
The latter would require fewer changes, e.g. no need for TypeDef::setLocation().

Copy link
Member Author

Choose a reason for hiding this comment

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

TypeDef instance in any case should have location because it may reference any type: typedef int size.

Struct, Union and Enum are not referenced directly, corresponding TypeDef is generated for each instance, so it is okay to use location from TypeDef.

It is possible to add location to them too, but in some sense TypeDef replaces actual type, and we store Struct, .. only for helper methods.

So when generator checks for unused external types it can go through all typedefs and check their location.
If a TypeDef is removed and it references a struct, the struct is also removed.

I am not sure if it was right choice, I can try to add location to Struct etc and see if it will make code more complicated.

structs.push_back(s);
std::shared_ptr<TypeDef> typeDef = getTypeDefWithName("struct_" + name);
if (typeDef) {
/* the struct type used to be opaque type, typeDef contains nullptr */
typeDef.get()->setType(s);
typeDef.get()->setLocation(location);
} else {
typeDefs.push_back(s->generateTypeDef());
typeDefs.push_back(s->generateTypeDef(std::move(location)));
}
}

void IR::addUnion(std::string name, std::vector<Field *> fields,
uint64_t maxSize) {
uint64_t maxSize, std::shared_ptr<Location> location) {
std::shared_ptr<Union> u =
std::make_shared<Union>(name, std::move(fields), maxSize);
unions.push_back(u);
std::shared_ptr<TypeDef> typeDef = getTypeDefWithName("union_" + name);
if (typeDef) {
/* the union type used to be opaque type, typeDef contains nullptr */
typeDef.get()->setType(u);
typeDef.get()->setLocation(location);
} else {
typeDefs.push_back(u->generateTypeDef());
typeDefs.push_back(u->generateTypeDef(std::move(location)));
}
}

Expand Down Expand Up @@ -171,6 +177,7 @@ void IR::generate(const std::string &excludePrefix) {
if (!generated) {
setScalaNames();
filterDeclarations(excludePrefix);
removeUnusedExternalTypes();
generated = true;
}
}
Expand Down Expand Up @@ -230,7 +237,7 @@ void IR::replaceTypeInTypeDefs(std::shared_ptr<Type> oldType,

template <typename T>
bool IR::isTypeUsed(const std::vector<T> &declarations,
std::shared_ptr<Type> type, bool stopOnTypeDefs) {
std::shared_ptr<Type> type, bool stopOnTypeDefs) const {
for (const auto &decl : declarations) {
if (decl->usesType(type, stopOnTypeDefs)) {
return true;
Expand All @@ -239,7 +246,7 @@ bool IR::isTypeUsed(const std::vector<T> &declarations,
return false;
}

bool IR::typeIsUsedOnlyInTypeDefs(std::shared_ptr<Type> type) {
bool IR::typeIsUsedOnlyInTypeDefs(const std::shared_ptr<Type> &type) const {
/* varDefines are not checked here because they are simply
* aliases for variables.*/
return !(
Expand All @@ -248,6 +255,11 @@ bool IR::typeIsUsedOnlyInTypeDefs(std::shared_ptr<Type> type) {
isTypeUsed(literalDefines, type, true));
}

bool IR::isTypeUsed(const std::shared_ptr<Type> &type) const {
return !(typeIsUsedOnlyInTypeDefs(type) &&
!isTypeUsed(typeDefs, type, false));
}

void IR::setScalaNames() {
/* Renaming according to Scala naming conventions
* should happen here */
Expand Down Expand Up @@ -353,3 +365,47 @@ IR::~IR() {
variables.clear();
varDefines.clear();
}

void IR::removeUnusedExternalTypes() {
Copy link
Member

@jonas jonas Jul 7, 2018

Choose a reason for hiding this comment

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

I don't think it makes sense to remove types, since we loose a lot of information. We need the external types to correctly reference types from other modules, like stdio.FILE. It would be better to update the generators to skip when !isInMainFile and then have them also prefix the module path when referencing an external type.

for (auto it = typeDefs.begin(); it != typeDefs.end();) {
std::shared_ptr<TypeDef> typeDef = *it;
std::shared_ptr<Location> location = typeDef->getLocation();
auto *sourceLocation = dynamic_cast<SourceLocation *>(location.get());
if (sourceLocation && !sourceLocation->isMainFile()) {
if (!isTypeUsed(typeDef)) {
removeStructOrUnionOrEnum(typeDef->getType());
it = typeDefs.erase(it);
} else {
++it;
}
} else {
++it;
}
}
}

template <typename T>
void IR::removeDeclaration(std::vector<std::shared_ptr<T>> &declarations,
T *declaration) {
for (auto it = declarations.begin(); it != declarations.end();) {
T *decl = (*it).get();
if (decl == declaration) {
it = declarations.erase(it);
} else {
++it;
}
}
}

void IR::removeStructOrUnionOrEnum(std::shared_ptr<Type> type) {
if (isInstanceOf<Struct>(type.get())) {
auto *s = dynamic_cast<Struct *>(type.get());
removeDeclaration(structs, s);
} else if (isInstanceOf<Union>(type.get())) {
auto *u = dynamic_cast<Union *>(type.get());
removeDeclaration(unions, u);
} else if (isInstanceOf<Enum>(type.get())) {
auto *e = dynamic_cast<Enum *>(type.get());
removeDeclaration(enums, e);
}
}
Loading