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

DynamicLinking.md: add notes about memory/table exports #234

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Sep 3, 2024

This is my attempt to capture the discussion in
#232

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

LGTM % comments

DynamicLinking.md Outdated Show resolved Hide resolved

On the other hand, non-PIE executables need to export these instance
resources as usual.

## Implementation Status
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can/should add something under this header for wasi now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean to update the implementation status section?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we mention LLVM here and emscripten, perhaps you could mention wasi-sdk too? i.e. what is the status there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

DynamicLinking.md Show resolved Hide resolved
export them as well. For that reason, for PIE executables, we don't
require these resources exported.

Shared libraries don't need to export these instance resoures either
Copy link
Member

Choose a reason for hiding this comment

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

"Shared libraries should also import these resources, since they share a memory and function table with the main module."

@sunfishcode
Copy link
Member

@sbc100 It looks like you approved this; is this ready to land?

require these resources exported.

Shared libraries don't need to export these resources either.
They should also import these resources, since they share the memory
Copy link
Member

Choose a reason for hiding this comment

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

Instead, they should..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % comments

@yamt yamt force-pushed the dynamic-linking-import branch 2 times, most recently from c155913 to faf6c9e Compare January 6, 2025 08:14
@yamt yamt force-pushed the dynamic-linking-import branch from faf6c9e to 96fc30b Compare January 6, 2025 08:15

```
-fPIC \
-Xlinker -pie \
Copy link
Member

Choose a reason for hiding this comment

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

I believe -pie is clang flag. No need to -Xlinker here.

-fPIC \
-shared \
-Xlinker --experimental-pic \
-Wl,--unresolved-symbols=import-dynamic
Copy link
Member

Choose a reason for hiding this comment

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

You can combine these two lines, or at least be consistent. -Xlinker and -Wl basically do the same things but -Wl allows you to specify more than one argument at time. e.g.:

-Wl,--experimental-pic,--unresolved-symbols=import-dynamic

Copy link
Member

Choose a reason for hiding this comment

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

I think -Wl, is the more widely used/recognized one, but I could be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we want --unresolved-symbols=import-dynamic here? I would hope that the default of requiring all symbols to be resolved would work in many cases.

Perhaps its best to just drop this section listing specific link options since there are many combinations that can work, and since this is all experimental it could becoming out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure we want --unresolved-symbols=import-dynamic here? I would hope that the default of requiring all symbols to be resolved would work in many cases.

i copy-and-pasted these options from my test scripts.
maybe it can be outdated and/or redundant.

Perhaps its best to just drop this section listing specific link options since there are many combinations that can work, and since this is all experimental it could becoming out of date.

ok. it makes sense. i will remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

-Xlinker --export=__stack_pointer \
-Xlinker --export=__heap_base \
-Xlinker --export=__heap_end \
-z stack-size=16384
Copy link
Member

Choose a reason for hiding this comment

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

No need to stack-size here right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants