This is an archive of the discontinued LLVM Phabricator instance.

Wasm entrypoint changes #3 (add --no-entry argument to LLD)
ClosedPublic

Authored by ncw on Dec 1 2017, 7:01 AM.

Details

Summary

Split out from D40559 as requested.

This adds a --no-entry argument to LLD, used to suppress the default _start entrypoint.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ncw created this revision.Dec 1 2017, 7:01 AM
sbc100 added inline comments.Dec 1 2017, 11:25 AM
test/wasm/data-layout.ll
3

Looks like this is not based off of master. Can you rebase?

wasm/Driver.cpp
220

Empty string is not a valid symbol name. Do you have some reason to believe it is?

282

I don't think you need to change this. Treating empty as not present seems fine to me.

ncw added inline comments.Dec 1 2017, 12:00 PM
test/wasm/data-layout.ll
3

It's based off D40559. I've got a pipeline of patches here!

In the title of the patches, I've tried to make it clear what order I'm expecting them to be merged in. That's solely to save work for whoever ends up committing them on my behalf, so they don't have to handle the merge conflicts.

Of course if D40559 is rejected, I can rebase this one, but I basically don't want to put extra admin on some else to manage my patches. Hope that's helpful!

wasm/Driver.cpp
282

I agree it wasn't an urgent change, but now it's done (at Dan's suggestion/request) I don't think it's doing any harm.

ruiu added a subscriber: ruiu.Dec 1 2017, 12:49 PM
ruiu added inline comments.
wasm/Driver.cpp
220

You guys want to ban this by the wasm spec, no? Allowing the empty string as a symbol name isn't beneficial but just confusing, I think.

221

A newly created StringRef is considered to have an empty string. We don't usually distinguish uninitialized StringRef from a StringRef that happens to have an empty string. So this distinction is too subtle. If you really need to distinguish the two, a better way of doing it is to use Optional<StringRef>.

(But I believe disallowing the empty symbol is a better approach though.)

ruiu added inline comments.Dec 1 2017, 12:57 PM
wasm/Driver.cpp
220

On second thought, ELF might allow the empty string as a symbol name, but lld (and perhaps a lot of other tools) don't handle it as a valid symbol name because it doesn't worth it. I don't think you want to handle that super edge case.

sbc100 added inline comments.Dec 1 2017, 2:44 PM
wasm/Driver.cpp
220

Agreed

282

If you want to represent an options stringref as distict from the empty string you can use llvm::Optional for that. Using .data() doesn't make sense here I don't think.

However, I agree with ruiu. I don't think want to support empty string as a value entry point, or even a symbol name.

ncw updated this revision to Diff 125553.Dec 5 2017, 9:26 AM
ncw marked 3 inline comments as done.

Feedback addressed.

  • Added check to disallow using empty string with --entry and --undefined
  • Got rid of use of null StringRef for Entry
  • Switched Entry to an Optional so code expresses intent more clearly by checking if (Config->Entry) rather than if (Config->Entry.empty())
sbc100 accepted this revision.Dec 5 2017, 7:22 PM

LGTM - with a couple more comments.

test/wasm/data-layout.ll
3

Is the --allow-undefined still needed? (I was thinking it might be there simply because this test doesn't have an entry point).

wasm/Config.h
38 ↗(On Diff #125553)

I still think its simpler just to use empty string to mean no entry and continue to assume that empty strings cannot be symbols. I'm don't feel super strongly about it though. I guess maybe we should agree to spec in Linking.md that symbols cannot be empty strings?

wasm/Writer.cpp
251

I think you'll need to rebase this since the symbol is now stored in the Config itself.

This revision is now accepted and ready to land.Dec 5 2017, 7:22 PM
ruiu added inline comments.Dec 5 2017, 7:24 PM
wasm/Config.h
38 ↗(On Diff #125553)

I prefer empty string too. It is consistent with other lld ports.

ncw marked 3 inline comments as done.Dec 6 2017, 4:22 AM

Oops, OK, I've removed the use of Optional<StringRef> for Config::Entry.

The diff is shorter now, and should hopefully be good to go.

test/wasm/data-layout.ll
3

Good spot, but it actually still is needed. Inputs/hello.ll uses puts without defining it.

wasm/Writer.cpp
251

I have rebased, but I don't think there are any relevant changes to Config? The stack-pointer has moved in there, but the entrypoint's Symbol isn't in Config.

ncw updated this revision to Diff 125701.Dec 6 2017, 4:26 AM
ncw marked an inline comment as done.

Updated with changes as requested (and rebased)

ruiu added inline comments.Dec 6 2017, 3:20 PM
wasm/Driver.cpp
222–225

Let's not too picky about "". In other ports, we don't check whether a given value is empty or not. If you pass --entry '', the linker should do whatever it is instructed to do. We allow users to shoot themselves in the foot. If they really want to use an empty string as an entry symbol name, they should be able to do that.

So, let's return Arg->getValue() without assigning it to Entry and check if it is empty string.

253–255

nit: I believe you can write

Config->Relocatable ? "" : "_start"

or at least

StringRef(Config->Relocatable ? "" : "_start")
295–296

Ditto -- if users want to force loading an empty symbol, we should do that.

326–328

You can now eliminate Sym

ncw updated this revision to Diff 125945.Dec 7 2017, 6:41 AM
ncw marked 4 inline comments as done.
ncw retitled this revision from Wasm entrypoint changes #3 (add --no-entry argument to LLD) APPLY AFTER D40559 to Wasm entrypoint changes #3 (add --no-entry argument to LLD).

Updated with Rui's changes (all pretty minor).

Rebased, OK to apply directly to master.

Thanks for being patient reviewing all these patches!

wasm/Driver.cpp
222–225

Using empty string as the entrypoint name still won't work - because we just got rid of the code that handled unset/empty string differently.

I've made the change you requested to get rid of the empty-string check, but just bear in mind it won't actually make empty-string usable, it just makes --entry '' a legal synonym for --no-entry.

ruiu accepted this revision.Dec 7 2017, 2:09 PM

LGTM

Thank you for making the change. As to the empty string handling, I wanted to make it consistent with other ports, and we in general don't write much code for edge cases to keep the code straightforward.

wasm/Driver.cpp
326–327

You can combine these two ifs.

sbc100 added inline comments.Dec 7 2017, 5:09 PM
wasm/Driver.cpp
328

It looks like this change also means that --allow-undefined no longer applies to '_start'. I'm not necessarily against that but is there a reason for this change?

ncw added inline comments.Dec 8 2017, 2:40 AM
wasm/Driver.cpp
328

Ah good spot, I should have included that tweak in the --undefined review.

There is a reason: a) consistency with what we decided for --undefined, where --allow-undefined also doesn't apply, and b) it just makes no sense at all to import the entrypoint - if _start isn't found from libc. I can't imagine a single developer would want it to be silently added as an import instead!

ncw updated this revision to Diff 126119.Dec 8 2017, 4:10 AM

Rebased again - with Rui's cosmetic tweak.

ncw marked an inline comment as done.Dec 8 2017, 4:11 AM
This revision was automatically updated to reflect the committed changes.