sbc100 (Sam Clegg)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2016, 10:22 AM (100 w, 2 d)

Recent Activity

Fri, Aug 17

sbc100 added a comment to D50721: [WebAssembly] Preserve function signatures during LTO.

Ping

Fri, Aug 17, 12:43 PM

Thu, Aug 16

sbc100 accepted D50859: [WebAssembly] Remove temporary workaround for function bitcasts.

Yay!

Thu, Aug 16, 12:00 PM

Wed, Aug 15

sbc100 added a comment to D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types.

Please ignore this change, I had thought we wanted size_t to match either int32_t or int64_t, but it turns out that isn't a requirement and that code that expects this to be true needs fixing.

Wed, Aug 15, 3:26 PM

Tue, Aug 14

sbc100 added a reviewer for D50729: [WebAssembly] Don't compress LEBs by default: yurydelendik.
Tue, Aug 14, 4:52 PM
sbc100 accepted D50728: [WebAssembly] Delete a specific push number from test expectations.
Tue, Aug 14, 12:19 PM
sbc100 updated the diff for D50729: [WebAssembly] Don't compress LEBs by default.
  • add check for -r
Tue, Aug 14, 12:15 PM
sbc100 added a comment to D50729: [WebAssembly] Don't compress LEBs by default.

Perhaps I should make this -z compress although I'm not sure what the policy is in using -z keywords vs regular flags.

Tue, Aug 14, 12:13 PM
sbc100 added a reviewer for D50729: [WebAssembly] Don't compress LEBs by default: ruiu.
Tue, Aug 14, 12:12 PM
sbc100 created D50729: [WebAssembly] Don't compress LEBs by default.
Tue, Aug 14, 12:12 PM
sbc100 added a reviewer for D50721: [WebAssembly] Preserve function signatures during LTO: ruiu.
Tue, Aug 14, 11:43 AM
sbc100 updated the diff for D50721: [WebAssembly] Preserve function signatures during LTO.
  • fix type mismatch
  • add test
Tue, Aug 14, 11:38 AM
sbc100 created D50721: [WebAssembly] Preserve function signatures during LTO.
Tue, Aug 14, 9:44 AM

Wed, Aug 8

sbc100 abandoned D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types.
Wed, Aug 8, 5:51 PM
sbc100 updated the diff for D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types.

remove debugging

Wed, Aug 8, 2:11 PM
sbc100 created D50477: WIP: Ensure that the type of size_t is represended as one of the fixed width types.
Wed, Aug 8, 2:10 PM
sbc100 added a comment to D50472: Fix wasm backend compilation on gcc 5.4: variable name cannot match class.

Hmm. I don't know the convention. Not a huge and of "The" prefix. LGTM otherwise.

Wed, Aug 8, 1:53 PM
sbc100 added a comment to D50424: [WebAssembly] Group rodata into a single output segment.

I think you are right about the optimized builds. One thing I thought we might want to preserve is that data objects from the same section should be grouped together contiguously.. but I think we can probably do that while at the same time putting it all in a single segment.

Wed, Aug 8, 11:02 AM
sbc100 added a comment to D40526: [WebAssembly] Change size_t to `unsigned long`.

Is this related to the issue reported in the thread here?

Wed, Aug 8, 10:55 AM
sbc100 added a comment to D40526: [WebAssembly] Change size_t to `unsigned long`.

Should we change int32_t as well? Or does the above code just need fixing? I'm a little concerned because this code is very portable which implies that WebAssembly might be doing something unusual here.

Wed, Aug 8, 9:41 AM
sbc100 added a comment to D40526: [WebAssembly] Change size_t to `unsigned long`.

I'm running into an issue with an internal codebase that assumes that size_t is equivalent to either int32_t or int64_t which this change invalidates.

Wed, Aug 8, 9:39 AM
sbc100 accepted D50387: [WASM] Fix overflow when reading custom section.

I agree this feels a bit awkward, but I prefer to custom bounds checking I think.

Wed, Aug 8, 8:36 AM

Tue, Aug 7

sbc100 added reviewers for D50424: [WebAssembly] Group rodata into a single output segment: fitzgen, ruiu.
Tue, Aug 7, 9:03 PM
sbc100 created D50424: [WebAssembly] Group rodata into a single output segment.
Tue, Aug 7, 9:02 PM
sbc100 added inline comments to D50387: [WASM] Fix overflow when reading custom section.
Tue, Aug 7, 3:48 PM
sbc100 updated the diff for D50395: [WebAssembly] Remove use of lld -flavor flag.
  • update tests
Tue, Aug 7, 11:53 AM
sbc100 added a reviewer for D50395: [WebAssembly] Remove use of lld -flavor flag: ruiu.
Tue, Aug 7, 9:56 AM
sbc100 updated the diff for D50395: [WebAssembly] Remove use of lld -flavor flag.
  • update test
Tue, Aug 7, 9:56 AM
sbc100 created D50395: [WebAssembly] Remove use of lld -flavor flag.
Tue, Aug 7, 9:55 AM
sbc100 accepted D49897: [WebAssembly] Force use of lld for test/Driver/wasm-toolchain.c(pp).

I see. This seems a little strange to me. I'm not sure it makes sense for CLANG_DEFAULT_LINKER to effect targets like WebAssembly that only have single supported linker. I'm OK with landing this now but I think we might need to look into a better solution.

Tue, Aug 7, 9:42 AM
sbc100 added inline comments to D50387: [WASM] Fix overflow when reading custom section.
Tue, Aug 7, 9:31 AM

Mon, Aug 6

sbc100 added a comment to D49897: [WebAssembly] Force use of lld for test/Driver/wasm-toolchain.c(pp).

But in the CL description you say "..when configuring clang to use a different linker by default". How is this possible? i.e. do you have a config where these tests are currently failing?

Mon, Aug 6, 11:32 AM
sbc100 added a reviewer for D50287: [WebAssembly] --export should fetch lazy symbols: dschuff.
Mon, Aug 6, 9:42 AM
sbc100 updated the summary of D50287: [WebAssembly] --export should fetch lazy symbols.
Mon, Aug 6, 9:42 AM

Fri, Aug 3

sbc100 updated the diff for D50287: [WebAssembly] --export should fetch lazy symbols.
  • update test
Fri, Aug 3, 5:18 PM
sbc100 added a reviewer for D50287: [WebAssembly] --export should fetch lazy symbols: ruiu.
Fri, Aug 3, 5:15 PM
sbc100 added a comment to D50287: [WebAssembly] --export should fetch lazy symbols.

Second attempt at this change since I think it makes sense. Otherwise emscripten would need to pass --export=foo and --undefined=foo for each symbol which seems redundant.

Fri, Aug 3, 5:15 PM
sbc100 created D50287: [WebAssembly] --export should fetch lazy symbols.
Fri, Aug 3, 5:14 PM
sbc100 added inline comments to D50279: [WebAssembly] Don't error when --undefined symbols are not found.
Fri, Aug 3, 4:51 PM
sbc100 updated the diff for D50279: [WebAssembly] Don't error when --undefined symbols are not found.
  • --undefined should work with -r
Fri, Aug 3, 4:50 PM
sbc100 updated the diff for D50279: [WebAssembly] Don't error when --undefined symbols are not found.
  • revert part
Fri, Aug 3, 4:43 PM
sbc100 updated the diff for D50279: [WebAssembly] Don't error when --undefined symbols are not found.
  • Handle --undefined like the ELF linker
Fri, Aug 3, 4:40 PM
sbc100 updated the diff for D50279: [WebAssembly] Don't error when --undefined symbols are not found.
  • Update comment
Fri, Aug 3, 4:05 PM
sbc100 retitled D50279: [WebAssembly] Don't error when --undefined symbols are not found from Don't error when --undefined symbols are not found to [WebAssembly] Don't error when --undefined symbols are not found.
Fri, Aug 3, 4:02 PM
sbc100 updated the summary of D50279: [WebAssembly] Don't error when --undefined symbols are not found.
Fri, Aug 3, 4:02 PM
sbc100 abandoned D50273: All lazy symbols to be exported with --export.

Ok, lets try that approach: https://reviews.llvm.org/D50279

Fri, Aug 3, 4:01 PM
sbc100 created D50279: [WebAssembly] Don't error when --undefined symbols are not found.
Fri, Aug 3, 4:00 PM
sbc100 added a comment to D50273: All lazy symbols to be exported with --export.

Oh, I didn't realize that. In that case perhaps we need to bring --undefined in line with the ELF linker?

Fri, Aug 3, 3:24 PM
sbc100 added a comment to D50273: All lazy symbols to be exported with --export.

A bit more detail: Emscripten has flag called "-s EXPORTED_FUNCTIONS=<list>". By default this list includes just main. But those symbols can either come from native code or from JS libraries, so they might not exist and link time. Ideally we would know which ones we need at link time and with --export=foo and --undefined=foo which would force all these symbols to exist. But right now we don't have that knowledge so we use --allow-undefined with --export=foo, which roughly equates to "export these symbols if they are found at link time but don't error out if they are not".

Fri, Aug 3, 3:23 PM
sbc100 added a comment to D50273: All lazy symbols to be exported with --export.

Emscripten uses --export in order to choose which symbols to export. It does this for main too with --export=main. We can't use --undefined in general since (for complicated reasons) some of these symbols might actually be missing at link time.

Fri, Aug 3, 3:19 PM
sbc100 added reviewers for D50273: All lazy symbols to be exported with --export: ruiu, dschuff.
Fri, Aug 3, 3:10 PM
sbc100 created D50273: All lazy symbols to be exported with --export.
Fri, Aug 3, 3:10 PM

Thu, Aug 2

sbc100 added a comment to D49147: Set Symbol::IsUsedInRegularObj in a consistent manor between COFF, ELF and wasm. NFC.

ping..

Thu, Aug 2, 10:43 AM
sbc100 abandoned D49344: [llvm-ar] Update help text.

rL338703 made this redundant

Thu, Aug 2, 10:42 AM
sbc100 updated the diff for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
  • update comment
Thu, Aug 2, 10:04 AM

Wed, Aug 1

sbc100 added a comment to D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.

PTAL

Wed, Aug 1, 3:29 PM
sbc100 updated the diff for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
  • remove logging
Wed, Aug 1, 3:23 PM
sbc100 added a reviewer for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass: jgravelle-google.
Wed, Aug 1, 3:22 PM
sbc100 retitled D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass from [WebAssembly] Handle return type conversions in FixFunctionBitcasts pass to [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
Wed, Aug 1, 3:22 PM
sbc100 updated the diff for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
Wed, Aug 1, 11:08 AM
sbc100 updated the diff for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
  • revert
Wed, Aug 1, 10:08 AM
sbc100 updated the diff for D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.

gererate runtime calls to abort rather than failing to compile
add more tests

Wed, Aug 1, 8:35 AM

Mon, Jul 30

sbc100 added a comment to D49897: [WebAssembly] Force use of lld for test/Driver/wasm-toolchain.c(pp).

How do you configure clang to use a different linker? It looks like getDefaultLinker() is hardcoded, but maybe I'm missing something.

Mon, Jul 30, 8:55 AM

Wed, Jul 25

sbc100 added a reviewer for D49825: [CMake] Don't use LIBCXXABI_ENABLE_STATIC option before its declared: aheejin.
Wed, Jul 25, 4:01 PM
sbc100 added a comment to D49573: [CMake] Option to control whether shared/static library is installed.

https://reviews.llvm.org/D49825 seems to fix it.

Wed, Jul 25, 4:00 PM
sbc100 added a reviewer for D49825: [CMake] Don't use LIBCXXABI_ENABLE_STATIC option before its declared: phosek.
Wed, Jul 25, 3:59 PM
sbc100 created D49825: [CMake] Don't use LIBCXXABI_ENABLE_STATIC option before its declared.
Wed, Jul 25, 3:59 PM
sbc100 added a comment to D49573: [CMake] Option to control whether shared/static library is installed.

FYI, this broke the WebAssembly waterfall, where the library is no longer being installed: https://wasm-stat.us/builders/linux/builds/34191

Wed, Jul 25, 3:36 PM

Mon, Jul 23

sbc100 added a comment to D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

Mon, Jul 23, 5:14 PM
sbc100 updated the diff for D49147: Set Symbol::IsUsedInRegularObj in a consistent manor between COFF, ELF and wasm. NFC.
  • remove overload
Mon, Jul 23, 5:09 PM
sbc100 added a comment to D49706: [WebAssembly] Add support for --whole-archive..

I did consider that, but the COFF version is slightly different. Perhaps as a followup to keep this change strictly wasm-only?

Mon, Jul 23, 4:30 PM
sbc100 added a reviewer for D49706: [WebAssembly] Add support for --whole-archive.: ruiu.
Mon, Jul 23, 4:27 PM
sbc100 updated the diff for D49706: [WebAssembly] Add support for --whole-archive..
  • revert
Mon, Jul 23, 4:27 PM
sbc100 created D49706: [WebAssembly] Add support for --whole-archive..
Mon, Jul 23, 4:22 PM
sbc100 updated the diff for D49147: Set Symbol::IsUsedInRegularObj in a consistent manor between COFF, ELF and wasm. NFC.
  • revert change to wasm addLazy
Mon, Jul 23, 3:43 PM
sbc100 added a comment to D49147: Set Symbol::IsUsedInRegularObj in a consistent manor between COFF, ELF and wasm. NFC.

ping..

Mon, Jul 23, 3:32 PM

Jul 19 2018

sbc100 accepted D49577: [WebAssembly] Disable a test that violates DR1696.

Do did enforcement of this rule increase recently? I wonder if they knew they would break existing code.

Jul 19 2018, 5:11 PM
sbc100 added a comment to D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.

When you say not all types are valid to cast to all others.. how would I know which are valid? And how would this code fail if they are invalid? Would llvm bail out during the call to getCastOpcode()?

Jul 19 2018, 5:08 PM
sbc100 added a comment to D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.

My goal here is not so much to make it "work" as to not produce invalid webassembly. i.e. it should compile, even it if fails at runtime to do anything useful/expected.

Jul 19 2018, 5:05 PM

Jul 18 2018

sbc100 created D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.
Jul 18 2018, 4:12 PM
sbc100 added a comment to D49446: [WebAssembly] Move .debug_line section address of dead function outside section range.

I'm still not sure I understand why writing zero here is not a good idea.

Jul 18 2018, 2:49 PM

Jul 17 2018

sbc100 updated the diff for D49147: Set Symbol::IsUsedInRegularObj in a consistent manor between COFF, ELF and wasm. NFC.
  • rebase
Jul 17 2018, 12:26 PM
sbc100 updated the diff for D49343: [WebAssembly] Fix archive member display in error messages.
  • rebase
Jul 17 2018, 12:21 PM
sbc100 added a comment to D49113: [WebAssemlby] Set IsUsedInRegularObj correctly for undefined data symbols.

Adding @jgravelle-google. Maybe @ruiu is OOO. This change I pretty small an non-controversial I think, not that I've explained it. These two lines should have been in the original LTO change.

Jul 17 2018, 10:43 AM
sbc100 added a reviewer for D49113: [WebAssemlby] Set IsUsedInRegularObj correctly for undefined data symbols: jgravelle-google.
Jul 17 2018, 10:42 AM

Jul 16 2018

sbc100 added a comment to D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.

I think we can still do that library option, with this as an intermediate step. This change has the nice property of not requiring an emscripten-side change.

Jul 16 2018, 3:55 PM
sbc100 updated the diff for D48838: Add wasm-ld to help text.
  • feedback
Jul 16 2018, 3:33 PM
sbc100 added inline comments to D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.
Jul 16 2018, 3:28 PM
sbc100 updated the diff for D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.
  • feedback
Jul 16 2018, 3:28 PM
sbc100 added a comment to D48744: [WebAssembly] Remove ELF file support..

Anybody feeling LG'ing this?

Jul 16 2018, 3:27 PM
sbc100 updated the diff for D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.
  • feedback
Jul 16 2018, 11:40 AM

Jul 14 2018

sbc100 added a reviewer for D49344: [llvm-ar] Update help text: ruiu.
Jul 14 2018, 12:38 PM
sbc100 created D49344: [llvm-ar] Update help text.
Jul 14 2018, 12:37 PM
sbc100 added a reviewer for D49343: [WebAssembly] Fix archive member display in error messages: ruiu.
Jul 14 2018, 12:12 PM
sbc100 updated the diff for D49343: [WebAssembly] Fix archive member display in error messages.
  • cleanup
Jul 14 2018, 12:12 PM
sbc100 created D49343: [WebAssembly] Fix archive member display in error messages.
Jul 14 2018, 12:05 PM

Jul 12 2018

sbc100 added a comment to D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.

I agree it that the other approach is more elegant. However, I decided to go with this one for now since:
(1) Its a smaller incremental change
(2) Hopefully all this code will be removed pretty soon anyway

Jul 12 2018, 12:36 PM
sbc100 updated the summary of D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.
Jul 12 2018, 12:02 PM
sbc100 added reviewers for D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation: dschuff, aheejin.
Jul 12 2018, 12:02 PM
sbc100 updated the diff for D49263: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to handle separate compilation.
  • cleanup
Jul 12 2018, 12:01 PM
sbc100 abandoned D49208: [WebAssembly] Update WebAssemblyLowerEmscriptenEHSjLj to support separate compilation..

Abandoning in favor of weak linking approach: https://reviews.llvm.org/D49263

Jul 12 2018, 12:00 PM