This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Tidy up handling of global symbol relocations
ClosedPublic

Authored by ncw on Mar 2 2018, 10:26 AM.

Details

Summary

Currently global relocations are using their own fixup kind, which is odd since it's just a uleb32. That's because we're not tracking the difference between data and global symbols, which would enable us to process the fixup in the same way as all the other fixups.

Also, we're using some target flags already (like 0x1 to mean "function") which are just defined inline in the code and not in a header!


These changes are preliminary to the next commit which will add TLS (__thread_pointer) support.

I'm new to this area of the codebase, this might be all wrong! I think it looks sensible so far though.

Diff Detail

Repository
rL LLVM

Event Timeline

ncw created this revision.Mar 2 2018, 10:26 AM
ncw added inline comments.Mar 2 2018, 10:29 AM
lib/MC/MCWasmStreamer.cpp
120 ↗(On Diff #136786)

This is the only bit I was really unsure about, in terms of whether it breaks anything... currently the "ELF_TypeObject" attribute is set for both WASM_SYMBOL_TYPE_DATA and WASM_SYMBOL_TYPE_GLOBAL symbols, which is a bit odd. It seems to come from higher-level code that just sets it by default, rather than because we really want that attribute there in our Wasm objects.

Looks good to me. @sunfish knows this stuff better than me. I guess the .s file form for -elf won't change at all (since we don't use globals there).. otherwise we could need to fix binaryen (which currently consumes .s).

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
91 ↗(On Diff #136786)

Other platforms seem to call this TOF. Any reason not to follow suit?

ncw added a comment.Mar 2 2018, 1:06 PM

Thanks for the review Sam, I hadn't added you to this one you wouldn't be bothered by all my commits!

Things are really pretty snowy here, which is unusual and nice, although quite inconvenient. Have a look at some pics of Britain in the news if you haven't, it's lovely and white.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
91 ↗(On Diff #136786)

If it's inconsistent, it's sheer ignorance! I think I did copy this name from one of the other targets, maybe the wrong one though.

I'll check and fix the name.

sbc100 added a comment.Mar 2 2018, 1:38 PM
In D44030#1025941, @ncw wrote:

Thanks for the review Sam, I hadn't added you to this one you wouldn't be bothered by all my commits!

Things are really pretty snowy here, which is unusual and nice, although quite inconvenient. Have a look at some pics of Britain in the news if you haven't, it's lovely and white.

Oh.. I must have been auto-cc'd by herald. I didn't notice it was just a CC.

I heard its cold there yes. We are also getting a lot of snow up in Tahoe which is great news for California.

ncw added a comment.Mar 12 2018, 8:40 AM

Ping - has anyone who's worked on this code got a chance to look at it this week? Thanks very much!

ncw added a comment.Mar 27 2018, 9:50 AM

Ping again...

ncw added a comment.Apr 11 2018, 6:26 AM

@sunfish, @dschuff, it's been another fortnight on this PR. If you don't want to look at it, or want to suggest another reviewer - that's fine, but it would be helpful to know, to get some feedback on whether I should by trying to get this in, or abandon it.

The main improvement in this PR is removing the strange WebAssembly::fixup_code_global_index fixup kind, and switching to using plain uleb32 fixups for global indexes, just the same as function index fixups currently do. Also I've pulled out some hardcoded constants into headers, and added an operand flag for globals to match the existing operand flag for function indexes (this is necessary to enable using the generic uleb32 fixup kind with globals).

sunfish accepted this revision.Jul 24 2018, 9:42 PM

First, I apologize, I'm not sure how I lost track of this patch and didn't see the pings.

The patch looks good to me. Adding @GLOBAL rather than inferring it from the fact that a symbol appears in a get_global/set_global seems reasonable.

include/llvm/MC/MCExpr.h
282 ↗(On Diff #136786)

Please vertically align the comment here.

This revision is now accepted and ready to land.Jul 24 2018, 9:42 PM
ncw updated this revision to Diff 158999.Aug 3 2018, 6:46 AM

Updated with Dan and Sam's review comments.

Thanks very much for looking at this!

ncw marked 2 inline comments as done.Aug 3 2018, 6:49 AM
This revision was automatically updated to reflect the committed changes.