Skip to content

Commit

Permalink
Fix bug with circular import detection (#563)
Browse files Browse the repository at this point in the history
  • Loading branch information
marcauberer authored May 25, 2024
1 parent 76926cf commit 0f31f2f
Show file tree
Hide file tree
Showing 10 changed files with 60 additions and 55 deletions.
9 changes: 5 additions & 4 deletions src/SourceFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ void SourceFile::concludeCompilation() {
dumpOutput(compilerOutput.typesString, "Type Registry", "type-registry.out");

// Print warning if verifier is disabled
if (parent == nullptr && cliOptions.disableVerifier) {
if (isMainFile && cliOptions.disableVerifier) {
const std::string warningMessage =
CompilerWarning(VERIFIER_DISABLED, "The LLVM verifier passes are disabled. Please use this cli option carefully.")
.warningMessage;
Expand Down Expand Up @@ -619,7 +619,8 @@ bool SourceFile::isAlreadyImported(const std::string &filePathSearch, // NOLINT(
if (std::filesystem::equivalent(filePath, filePathSearch))
return true;
// Check parent recursively
return parent != nullptr && parent->isAlreadyImported(filePathSearch, circle);
const auto pred = [&](const SourceFile *d) { return d->isAlreadyImported(filePathSearch, circle); }; // NOLINT(*-no-recursion)
return std::ranges::any_of(dependants, pred);
}

SourceFile *SourceFile::requestRuntimeModule(RuntimeModule runtimeModule) {
Expand Down Expand Up @@ -692,7 +693,7 @@ void SourceFile::collectAndPrintWarnings() { // NOLINT(misc-no-recursion)
}

const SourceFile *SourceFile::getRootSourceFile() const { // NOLINT(misc-no-recursion)
return parent == nullptr ? this : parent->getRootSourceFile();
return isMainFile ? this : parent->getRootSourceFile();
}

bool SourceFile::isRT(RuntimeModule runtimeModule) const {
Expand Down Expand Up @@ -761,7 +762,7 @@ void SourceFile::dumpOutput(const std::string &content, const std::string &capti
}

void SourceFile::visualizerPreamble(std::stringstream &output) const {
if (parent == nullptr)
if (isMainFile)
output << "digraph {\n rankdir=\"TB\";\n";
else
output << "subgraph {\n";
Expand Down
4 changes: 2 additions & 2 deletions src/irgenerator/DebugInfoGenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,13 @@ llvm::DICompositeType *DebugInfoGenerator::generateCaptureStructDebugInfo(const
std::vector<SymbolTableEntry *> fieldEntries;
QualTypeList fieldSymbolTypes;
for (const auto &[_, capture] : captures) {
QualType captureType = capture.capturedEntry->getQualType();
QualType captureType = capture.capturedSymbol->getQualType();

// Capture by reference
if (capture.getMode() == BY_REFERENCE)
captureType = captureType.toRef(node);

fieldEntries.push_back(capture.capturedEntry);
fieldEntries.push_back(capture.capturedSymbol);
fieldSymbolTypes.push_back(captureType);
fieldTypes.push_back(captureType.toLLVMType(irGenerator->sourceFile));
}
Expand Down
24 changes: 12 additions & 12 deletions src/irgenerator/GenValues.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ std::any IRGenerator::visitLambdaFunc(const LambdaFuncNode *node) {
// Pop capture addresses
if (hasCaptures)
for (const auto &[_, capture] : captures)
capture.capturedEntry->popAddress();
capture.capturedSymbol->popAddress();

// Conclude debug info for function
diGenerator.concludeFunctionDebugInfo();
Expand Down Expand Up @@ -686,7 +686,7 @@ std::any IRGenerator::visitLambdaProc(const LambdaProcNode *node) {
// Pop capture addresses
if (hasCaptures)
for (const auto &[_, capture] : captures)
capture.capturedEntry->popAddress();
capture.capturedSymbol->popAddress();

// Conclude debug info for function
diGenerator.concludeFunctionDebugInfo();
Expand Down Expand Up @@ -828,7 +828,7 @@ std::any IRGenerator::visitLambdaExpr(const LambdaExprNode *node) {
// Pop capture addresses
if (hasCaptures)
for (const std::pair<const std::string, Capture> &capture : captures)
capture.second.capturedEntry->popAddress();
capture.second.capturedSymbol->popAddress();

// Conclude debug info for function
diGenerator.concludeFunctionDebugInfo();
Expand Down Expand Up @@ -873,16 +873,16 @@ llvm::Value *IRGenerator::buildFatFctPtr(Scope *bodyScope, llvm::Type *capturesS
assert(captures.size() == 1);
const Capture &capture = captures.begin()->second;
if (capture.getMode() == BY_VALUE) {
llvm::Type *varType = capture.capturedEntry->getQualType().toLLVMType(sourceFile);
capturesPtr = insertLoad(varType, capture.capturedEntry->getAddress());
llvm::Type *varType = capture.capturedSymbol->getQualType().toLLVMType(sourceFile);
capturesPtr = insertLoad(varType, capture.capturedSymbol->getAddress());
} else {
capturesPtr = capture.capturedEntry->getAddress();
capturesPtr = capture.capturedSymbol->getAddress();
}
} else {
capturesPtr = insertAlloca(capturesStructType, CAPTURES_PARAM_NAME);
size_t captureIdx = 0;
for (const auto &[_, capture] : bodyScope->symbolTable.captures) {
const SymbolTableEntry *capturedEntry = capture.capturedEntry;
const SymbolTableEntry *capturedEntry = capture.capturedSymbol;
// Get address or value of captured variable, depending on the capturing mode
llvm::Value *capturedValue = capturedEntry->getAddress();
assert(capturedValue != nullptr);
Expand Down Expand Up @@ -917,14 +917,14 @@ llvm::Type *IRGenerator::buildCapturesContainerType(const CaptureMap &captures)

// If we have only one capture that is a ptr, we can just use that ptr type
const Capture &capture = captures.begin()->second;
if (captures.size() == 1 && (capture.capturedEntry->getQualType().isPtr() || capture.getMode() == BY_REFERENCE))
if (captures.size() == 1 && (capture.capturedSymbol->getQualType().isPtr() || capture.getMode() == BY_REFERENCE))
return builder.getPtrTy();

// Create captures struct type
std::vector<llvm::Type *> captureTypes;
for (const auto &[_, c] : captures) {
if (c.getMode() == BY_VALUE)
captureTypes.push_back(c.capturedEntry->getQualType().toLLVMType(sourceFile));
captureTypes.push_back(c.capturedSymbol->getQualType().toLLVMType(sourceFile));
else
captureTypes.push_back(builder.getPtrTy());
}
Expand All @@ -935,10 +935,10 @@ void IRGenerator::unpackCapturesToLocalVariables(const CaptureMap &captures, llv
assert(!captures.empty());
// If we have only one capture that is a ptr, we can just load the ptr
const Capture &capture = captures.begin()->second;
if (captures.size() == 1 && (capture.capturedEntry->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) {
if (captures.size() == 1 && (capture.capturedSymbol->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) {
// Interpret capturesPtr as ptr to the first and only capture
llvm::Value *captureAddress = val;
capture.capturedEntry->pushAddress(captureAddress);
capture.capturedSymbol->pushAddress(captureAddress);
// Generate debug info
diGenerator.generateLocalVarDebugInfo(capture.getName(), captureAddress);
} else {
Expand All @@ -949,7 +949,7 @@ void IRGenerator::unpackCapturesToLocalVariables(const CaptureMap &captures, llv
for (const auto &[name, c] : captures) {
const std::string valueName = c.getMode() == BY_REFERENCE ? name + ".addr" : name;
llvm::Value *captureAddress = insertStructGEP(structType, capturesPtr, captureIdx, valueName);
c.capturedEntry->pushAddress(captureAddress);
c.capturedSymbol->pushAddress(captureAddress);
// Generate debug info
diGenerator.generateLocalVarDebugInfo(c.getName(), captureAddress);
captureIdx++;
Expand Down
8 changes: 4 additions & 4 deletions src/symboltablebuilder/Capture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace spice::compiler {

Capture::Capture(SymbolTableEntry *entry) : capturedEntry(entry) {
Capture::Capture(SymbolTableEntry *entry) : capturedSymbol(entry) {
// Set the capture mode depending on the symbol type
// All types with guaranteed size <= 64 bit are captured by value, all others by reference.
captureMode = entry->getQualType().isOneOf({TY_STRUCT, TY_INTERFACE}) ? BY_REFERENCE : BY_VALUE;
Expand All @@ -15,7 +15,7 @@ Capture::Capture(SymbolTableEntry *entry) : capturedEntry(entry) {
*
* @return Capture name or symbol name if no capture name was set
*/
std::string Capture::getName() const { return capturedEntry->name; }
std::string Capture::getName() const { return capturedSymbol->name; }

/**
* Set the access type of this capture.
Expand All @@ -36,7 +36,7 @@ void Capture::setAccessType(CaptureAccessType captureAccessType) {
*
* @return Capture mode
*/
CaptureMode Capture::getMode() const { return captureMode; }
CapturePassMode Capture::getMode() const { return captureMode; }

/**
* Stringify the current capture to a human-readable form. Used to dump whole symbol tables with their contents.
Expand All @@ -52,7 +52,7 @@ CaptureMode Capture::getMode() const { return captureMode; }
*/
nlohmann::ordered_json Capture::toJSON() const {
nlohmann::json result;
result["name"] = capturedEntry->name;
result["name"] = capturedSymbol->name;
result["accessType"] = accessType == READ_ONLY ? "READ_ONLY" : "READ_WRITE";
result["mode"] = captureMode == BY_VALUE ? "BY_VALUE" : "BY_REFERENCE";
return result;
Expand Down
8 changes: 4 additions & 4 deletions src/symboltablebuilder/Capture.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

namespace spice::compiler {

enum CaptureMode : uint8_t {
enum CapturePassMode : uint8_t {
BY_VALUE,
BY_REFERENCE,
};
Expand All @@ -28,16 +28,16 @@ class Capture {
// Public methods
[[nodiscard]] std::string getName() const;
void setAccessType(CaptureAccessType captureAccessType);
[[nodiscard]] CaptureMode getMode() const;
[[nodiscard]] CapturePassMode getMode() const;
[[nodiscard]] nlohmann::ordered_json toJSON() const;

// Public members
SymbolTableEntry *capturedEntry;
SymbolTableEntry *capturedSymbol;

private:
// Members
CaptureAccessType accessType = READ_ONLY;
CaptureMode captureMode = BY_VALUE;
CapturePassMode captureMode = BY_VALUE;
};

} // namespace spice::compiler
2 changes: 1 addition & 1 deletion src/symboltablebuilder/SymbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ SymbolTableEntry *SymbolTable::lookupStrict(const std::string &name) {
return &symbols.at(name);
// Check if a capture with this name exists in this scope
if (captures.contains(name))
return captures.at(name).capturedEntry;
return captures.at(name).capturedSymbol;
// Otherwise, return a nullptr
return nullptr;
}
Expand Down
47 changes: 25 additions & 22 deletions src/symboltablebuilder/TypeSpecifiers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,44 @@
#include "TypeSpecifiers.h"

#include <exception/CompilerError.h>
#include <symboltablebuilder/Type.h>
#include <symboltablebuilder/TypeChain.h>

namespace spice::compiler {

/**
* Get default type specifiers for a given super type
*
* @param superType Super type
* @return Default type specifiers
*/
TypeSpecifiers TypeSpecifiers::of(uint16_t superType) {
switch (superType) {
case TY_DOUBLE: // fall-through
case TY_INT: // fall-through
case TY_SHORT: // fall-through
case TY_LONG:
return {/*const*/ false, /*signed*/ true, /*unsigned*/ false, /*heap*/ false};
case TY_BYTE: // fall-through
case TY_CHAR: // fall-through
case TY_STRING: // fall-through
case TY_BOOL: // fall-through
case TY_PTR: // fall-through
case TY_REF: // fall-through
case TY_ARRAY: // fall-through
case TY_BYTE: // fall-through
case TY_CHAR: // fall-through
case TY_STRING: // fall-through
case TY_BOOL: // fall-through
case TY_PTR: // fall-through
case TY_REF: // fall-through
case TY_ARRAY: // fall-through
case TY_STRUCT: // fall-through
case TY_INTERFACE: // fall-through
case TY_FUNCTION: // fall-through
case TY_PROCEDURE:
return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false};
case TY_GENERIC:
// Generics must be non-signed and non-unsigned at the same time to ensure a proper function matching
return {/*const*/ false, /*signed*/ false, /*unsigned*/ false, /*heap*/ false};
case TY_STRUCT: // fall-through
case TY_INTERFACE:
return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false};
case TY_ENUM: // fall-through
case TY_ALIAS:
return {/*const*/ true, /*signed*/ false, /*unsigned*/ true, /*heap*/ false};
case TY_FUNCTION: // fall-through
case TY_PROCEDURE:
return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false};
case TY_ENUM: // fall-through
case TY_ALIAS: // fall-through
case TY_IMPORT:
return {/*const*/ true, /*signed*/ false, /*unsigned*/ true, /*heap*/ false};
case TY_DYN:
case TY_INVALID:
case TY_DYN: // fall-through
case TY_INVALID: // fall-through
case TY_UNRESOLVED:
// Return all-false specifiers to not match anything
return {/*const*/ false, /*signed*/ false, /*unsigned*/ false, /*heap*/ false};
Expand All @@ -47,7 +50,7 @@ TypeSpecifiers TypeSpecifiers::of(uint16_t superType) {
}

/**
* Merge two type specifiers. If possible, prefer the opposite of the default of the super type
* Merge two sets of type specifiers. If possible, prefer the opposite of the default of the super type
*
* @param other Other type specifiers object
* @return Merged specifiers object
Expand All @@ -69,14 +72,14 @@ TypeSpecifiers TypeSpecifiers::merge(const TypeSpecifiers &other) const {
}

/**
* Check if two type specifiers match
* Check if two sets of type specifiers match
*
* @param otherSpecifiers The rhs specifiers
* @param allowConstify Match when the types are the same, but the lhs type is more const restrictive than the rhs type
* @return Matching or not
*/
bool TypeSpecifiers::match(TypeSpecifiers otherSpecifiers, bool allowConstify) const {
TypeSpecifiers thisSpecifiers = *this;
const TypeSpecifiers thisSpecifiers = *this;

// If allowConstify is enabled, only allow to match lhs=const and rhs=non-const
if (allowConstify && thisSpecifiers.isConst && !otherSpecifiers.isConst)
Expand Down
9 changes: 5 additions & 4 deletions src/typechecker/TypeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1900,10 +1900,11 @@ bool TypeChecker::visitMethodCall(FctCallNode *node, Scope *structScope, QualTyp

// Retrieve field entry
SymbolTableEntry *fieldEntry = structScope->lookupStrict(identifier);
if (!fieldEntry)
if (!fieldEntry) {
const std::string name = data.thisType.getBase().getName(false, true);
SOFT_ERROR_BOOL(node, ACCESS_TO_NON_EXISTING_MEMBER,
"The type " + data.thisType.getBase().getName(false) + " does not have a member with the name '" +
identifier + "'")
"The type '" + name + "' does not have a member with the name '" + identifier + "'")
}
if (!fieldEntry->getQualType().getBase().isOneOf({TY_STRUCT, TY_INTERFACE}))
SOFT_ERROR_BOOL(node, INVALID_MEMBER_ACCESS,
"Cannot call a method on '" + identifier + "', since it is no struct or interface")
Expand Down Expand Up @@ -2519,7 +2520,7 @@ bool TypeChecker::checkAsyncLambdaCaptureRules(LambdaBaseNode *node, const Lambd

// Check for the capture rules
const Capture &capture = captures.begin()->second;
if (captures.size() > 1 || !capture.capturedEntry->getQualType().isPtr() || capture.getMode() != BY_VALUE) {
if (captures.size() > 1 || !capture.capturedSymbol->getQualType().isPtr() || capture.getMode() != BY_VALUE) {
const char *warningMessage =
"Async lambdas can only capture one pointer by value without storing captures in the caller stack frame, which can lead "
"to bugs due to references, outliving the validity scope of the referenced variable.";
Expand Down
2 changes: 1 addition & 1 deletion src/util/CommonUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ bool CommonUtil::isValidMangledName(const std::string &mangledName) {
/**
* Generate a circular import message from the given source files
*
* @param sourceFiles Source files building the circular dependency chain
* @param sourceFiles Source files that form the circular dependency chain
* @return Error message
*/
std::string CommonUtil::getCircularImportMessage(const std::vector<const SourceFile *> &sourceFiles) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Unresolved soft errors: There are unresolved errors. Please fix them and recompile.

[Error|Semantic] ./source.spice:9:5:
Access to non-existing member: The type TestStruct does not have a member with the name 'nonExisting'
Access to non-existing member: The type 'TestStruct' does not have a member with the name 'nonExisting'

9 ts.f3.f3.nonExisting.testFunc();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 comments on commit 0f31f2f

Please sign in to comment.