Skip to content

[lld][WebAssembly] Allow --trace-symbol to work with symbols with custom import names #96119

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: main
Choose a base branch
from
Open

[lld][WebAssembly] Allow --trace-symbol to work with symbols with custom import names #96119

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
14 changes: 13 additions & 1 deletion lld/test/wasm/trace-symbol.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
# Test -y symbol and -trace-symbol=symbol

# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %p/Inputs/ret32.s -o %t.ret32.o
# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.start.o %s
# RUN: wasm-ld -o %t.wasm %t.start.o %t.ret32.o -y ret32 -y _start | FileCheck %s -check-prefix=BOTH
# RUN: wasm-ld -o %t.wasm %t.ret32.o %t.start.o -y ret32 -y _start | FileCheck %s -check-prefix=REVERSED

# check alias
# check long argument form
# RUN: wasm-ld -o %t.wasm %t.start.o %t.ret32.o -trace-symbol=_start | FileCheck %s -check-prefixes=JUST-START

# check import names can be traced
# RUN: wasm-ld -o %t.wasm %t.start.o %t.ret32.o -trace-symbol=foo_external | FileCheck %s -check-prefixes=IMPORT-NAME

.functype ret32 (f32) -> (i32)

.functype foo_import () -> ()
.import_name foo_import, foo_external

.globl _start
_start:
.functype _start () -> ()
f32.const 0.0
call ret32
drop
call foo_import
end_function

# BOTH: start.o: definition of _start
Expand All @@ -26,3 +35,6 @@ _start:

# JUST-START: start.o: definition of _start
# JUST-START-NOT: ret32

# IMPORT-NAME: start.o: reference to foo_external
# IMPORT-NAME-NOT: ret32
37 changes: 31 additions & 6 deletions lld/wasm/SymbolTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,15 @@ static void setImportAttributes(T *existing,
}
}

static void traceImport(std::optional<StringRef> importName, InputFile *file) {
if (importName.has_value()) {
auto name = importName.value();
if (symtab->isTraced(name)) {
printTraceSymbolUndefined(name, file);
}
}
}

Symbol *SymbolTable::addUndefinedFunction(StringRef name,
std::optional<StringRef> importName,
std::optional<StringRef> importModule,
Expand All @@ -624,6 +633,7 @@ Symbol *SymbolTable::addUndefinedFunction(StringRef name,
printTraceSymbolUndefined(name, file);
Copy link
Member

Choose a reason for hiding this comment

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

We run printTraceSymbolUndefined here and possibly again when wasInserted is true below. Does this mean s->traced and wasInserted are never true at the same time?
Would it be clear if we check two conditions at a single place to run this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, s->traced depends only on the command line flags and will be true if and only if --trace-symbol=foo is passed for a given symbol foo. It can be true or false independently of everything else going on here.

I'm not sure what you mean by "and then again". I only see a single call to printTraceSymbolUndefined in this function.

Copy link
Member

Choose a reason for hiding this comment

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

This function contains a call to traceImport which calls printTraceSymbolUndefined. So I guess if s->traced and wasInserted are both true it can be called twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes in that case you potentially see a trace entry for the "name" and the "import name". Not that those are normally different but if you happened to trace both then I would expect two trace entires in the log yes.


auto replaceSym = [&]() {
traceImport(importName, file);
Copy link
Member

Choose a reason for hiding this comment

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

This replaceSym also seems to be called when wasInserted was not true below:

      // If the existing undefined functions is not called directly then let
      // this one take precedence.  Otherwise the existing function is either
      // directly called or defined, in which case we need a function variant.
      if (existingUndefined && !existingUndefined->isCalledDirectly)
        replaceSym();
      else if (getFunctionVariant(s, sig, file, &s))
        replaceSym();

Is that OK to run this again in this case?

replaceSymbol<UndefinedFunction>(s, name, importName, importModule, flags,
file, sig, isCalledDirectly);
};
Expand Down Expand Up @@ -666,6 +676,7 @@ Symbol *SymbolTable::addUndefinedFunction(StringRef name,
replaceSym();
}
if (existingUndefined) {
traceImport(importName, file);
setImportAttributes(existingUndefined, importName, importModule, flags,
file);
if (isCalledDirectly)
Expand Down Expand Up @@ -718,10 +729,11 @@ Symbol *SymbolTable::addUndefinedGlobal(StringRef name,
if (s->traced)
printTraceSymbolUndefined(name, file);

if (wasInserted)
if (wasInserted) {
traceImport(importName, file);
Copy link
Member

Choose a reason for hiding this comment

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

traceImport sounds like telling the program to start tracing the import, which doesn't seem to be the case; it just prints the import.

By the way, I'm not very familiar with the code structure here so I might be missing something, but should we call printTraceSymbolUndefined and traceImport separately from several places? Can we do both in one function and preferably one place, when importName is not null?

replaceSymbol<UndefinedGlobal>(s, name, importName, importModule, flags,
file, type);
else if (auto *lazy = dyn_cast<LazySymbol>(s))
} else if (auto *lazy = dyn_cast<LazySymbol>(s))
lazy->extract();
else if (s->isDefined())
checkGlobalType(s, file, type);
Expand All @@ -744,10 +756,11 @@ Symbol *SymbolTable::addUndefinedTable(StringRef name,
if (s->traced)
printTraceSymbolUndefined(name, file);

if (wasInserted)
if (wasInserted) {
traceImport(importName, file);
replaceSymbol<UndefinedTable>(s, name, importName, importModule, flags,
file, type);
else if (auto *lazy = dyn_cast<LazySymbol>(s))
} else if (auto *lazy = dyn_cast<LazySymbol>(s))
lazy->extract();
else if (s->isDefined())
checkTableType(s, file, type);
Expand All @@ -770,10 +783,11 @@ Symbol *SymbolTable::addUndefinedTag(StringRef name,
if (s->traced)
printTraceSymbolUndefined(name, file);

if (wasInserted)
if (wasInserted) {
traceImport(importName, file);
replaceSymbol<UndefinedTag>(s, name, importName, importModule, flags, file,
sig);
else if (auto *lazy = dyn_cast<LazySymbol>(s))
} else if (auto *lazy = dyn_cast<LazySymbol>(s))
lazy->extract();
else if (s->isDefined())
checkTagType(s, file, sig);
Expand Down Expand Up @@ -937,9 +951,20 @@ bool SymbolTable::getFunctionVariant(Symbol* sym, const WasmSignature *sig,
// Set a flag for --trace-symbol so that we can print out a log message
// if a new symbol with the same name is inserted into the symbol table.
void SymbolTable::trace(StringRef name) {
tracingEnabled = true;
symMap.insert({CachedHashStringRef(name), -1});
}

bool SymbolTable::isTraced(StringRef name) {
// Early exit in the default case to avoid symbol hashmap lookup.
if (!tracingEnabled)
return false;
auto it = symMap.find(CachedHashStringRef(name));
if (it == symMap.end())
return false;
return it->second == -1 || symVector[it->second]->traced;
}

void SymbolTable::wrap(Symbol *sym, Symbol *real, Symbol *wrap) {
// Swap symbols as instructed by -wrap.
int &origIdx = symMap[CachedHashStringRef(sym->getName())];
Expand Down
5 changes: 5 additions & 0 deletions lld/wasm/SymbolTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,12 @@ class SymbolTable {

void trace(StringRef name);

bool isTraced(StringRef name);

Symbol *addSharedFunction(StringRef name, uint32_t flags, InputFile *file,
const WasmSignature *sig);
Symbol *addSharedData(StringRef name, uint32_t flags, InputFile *file);

Symbol *addDefinedFunction(StringRef name, uint32_t flags, InputFile *file,
InputFunction *function);
Symbol *addDefinedData(StringRef name, uint32_t flags, InputFile *file,
Expand Down Expand Up @@ -135,6 +138,8 @@ class SymbolTable {

// For LTO.
std::unique_ptr<BitcodeCompiler> lto;

bool tracingEnabled = false;
};

extern SymbolTable *symtab;
Expand Down
Loading