- User Since
- Jan 7 2013, 9:35 AM (267 w, 2 d)
Fri, Feb 16
Yeah come to think of it, I thought we just failed isel for blockaddress?
Mon, Feb 12
Fri, Feb 9
- My reading of the C standard is that using a prototype less declaration and defining that as a vararg functions is invalid in C. If you find bugs in this area we should fix them.
Interesting, I'll look closer at that. We haven't gotten any complaints about it so far.
- This patch simply removes all prototype less functions which is not fine.
Sorry, in case I wasn't clear, I agree that these tests are valid C and we should not change them.
- The llvm IR coming out of clang appears to mark all prototype less functions as varargs which from what I suspect makes it impossible to implement varargs different from prototype-less functions. But that is a problem in clang/IR, it doesn't change the fact that the tests are valid C.
Yeah, you can sort of cheat because in C, vararg functions must have at least one non-vararg parameter (at least AFAIK no C standard implemented by Clang allows that). So you can tell a real vararg function declare i32 @foo(i32, ...) from a non-prototype function declare i32 @foo(...). I suspect that only works for C though.
I'll just add that what we do in WebAssembly right now for unprototyped functions is to codegen them using the fixed-arg calling convention; at link-time if there are any mismatches between the definition and any calls, then the program has UB, and the resulting WebAssembly module will be invalid. I think this currently has the problem that if the definition does turn out to be vararg, then we have generated an invalid module even though the program is correct (I consider that a bug or shortcoming in our toolchain though). In any case, we do aim to support correct programs with prototypeless functions. If the test suite has non-vararg cases where there are mismatches between the callsites and/or the definition (I have seen that in some other test/benchmark suites), then even though that works on most X86 and ARM ABIs that I know of it's still UB and I'd be in favor of removing it from LLVM's test suite.
Thu, Feb 8
I like this idea, did you abandon this CL in favor of something else?
Mon, Feb 5
So, it turns out that this actually does change functionality on targets (such as WebAssembly) that use the expansion for unordered comparisons because it tries inverting the condition before falling all the way through to line 1680. This actually results in better code for wasm; see https://reviews.llvm.org/rL324305
@sunfish This is really just a change-detector test for the cond code expansion behavior that changed in https://reviews.llvm.org/D42788. Should we consider loosening it so that it just tests that we used the expansion? OTOH it's a bit concerning that that change didn't affect any other tests (!)
Fri, Feb 2
I do like the idea of having a linker-generated symbol instead of metadata for _end (or _wasm_end or whatever) and it's something that is unlikely to be broken in the future. So I think we should do that. But I do agree that specifying all these things would limit our ability to improve the layout in the future, especially wrt the order of the static/heap/stack bits. Since we have more constraints on our memory space than ELF (e.g. expanding only at the top instead of mmap), we might want to retain that flexibility, or even make things more dynamic or configurable, which would probably argue against matching the behavior of ELF here. We could consider having separate start/end markers for particular sections instead of having implicit ordering, if there's a use case for that.
Wed, Jan 31
Tue, Jan 30
I think LLD should probably default to no gc, and in general to minimal/most simple options, and the smarts should be in the driver. I think this matches other targets, and I think it also matches (or at least, should match) the way we have options the frontend/backend vs the driver; i.e. IIRC the driver has logic to pass -ffunction-sections and other codegen options to the frontend. So basically all the special logic goes in one place.
Tue, Jan 23
LGTM FTR, as long as we coordinate when this lands with emscripten.
I was going to say that most llc tests in LLVM don't use mtriple and that we should be consistent with that, but a quick grep suggests that something like 65% of the RUN lines with llc actually do contain mtriple. So I guess we could go either way. I think we still probably want to keep the triple in the bitcode (it is more convenient to run just llc, but only marginally so), in which case mtriple would be redundant. But I don't have strong opinions.
Jan 22 2018
These are just the compile failures :) There's also https://github.com/WebAssembly/waterfall/pull/316/files#diff-a6006086cef2fc4f685647d1fe006766
Jan 19 2018
- use StringRef::withNullAsEmpty() and clang-format
Jan 18 2018
- fix build
Jan 17 2018
- move types into anon namespace (and static stuff too just for consistency
I don't see that. How the underlying system allocates memory, and how it's split up for application use, don't have to relate at all. Exactly the same reasoning would say, "well x86 uses 4K pages so it doesn't make sense for malloc to subdivide that for a 10-byte allocation". I do agree though that it's odd/unusual and also awkward for mmap not to reflect the kernel's page size - but odd doesn't make it wrong, or inefficient.
Ping! This refactoring makes less work for anyone changing libcalls... any review takers?
- Merge branch 'master' into wasm_rtlib_def
- fix conflict
It seems like the real issue is that because it's meant for Linux, musl assumes the existence of Linux's mmap. On traditional architectures, there's a machine page size, which is also the granularity of mmap on Linux (technically you can have some kinds of mappings that don't end on page boundaries but those pages will just be fragmented). There's really no reason to do anything not on page boundaries, adn 4k is a pretty small page. The with wasm is that there's really no such thing as mmap for wasm (or I should say, for any wasm embedding, since mmap is an OS feature rather than an architecture feature). mmap is a strange swiss army knife, and currently the only functionality is to grow the memory at the end; If and when additional functionality is added (file mapping, protection, unmapped space, etc) that would be as separate calls. So mmap must always be emulated and if it is emulated then it's really just another layer of allocation. To me it makes much more sense to have one allocator (malloc) which knows directly about how wasm memory is managed (instead of assuming mmap's behavior which will always be a fiction, at least in JS-wasm). Since the real behavior is more or less brk() it makes sense to use that logic in existing allocators.
Likewise, because memory can never be allocated from the underlying system in increments of less than 64k, it doesn't really make much sense to pretend otherwise by implementing an mmap for malloc to use.
Jan 16 2018
Couldn't this just be in crtbegin.c? Then there wouldn't have to be custom linker logic for it.
Jan 11 2018
LGTM, but does it need to be rebased after the -allow-undefined-file change?
Cool, let's go with D41937, it has a proper diagnostic, and a test!
Jan 10 2018
The code LGTM. If there are objections to the scheme overall they can go in tool-conventions.
Jan 9 2018
Coming back to this patch, in order to remove the tight coupling between the arrays in WebAssemblyRuntimeLibcallSignatures.cpp and the list of libcalls in the def file.
- fix conflicts
- Merge branch 'master' into wasm_rtlib_def
Dec 15 2017
Dec 7 2017
Dec 5 2017
Nov 29 2017
Nov 28 2017
tl;dr: I think I agree with @sunfish
Nov 9 2017
I'm fine with making this explicit for now. I'm not sure I'd yet go so far as to say that the goal is not to support comdats. We should definitely investigate how C++ and its debug info are supported in mach-o, but it may be that comdats are better than whatever they do. In general in the absence of good reasons (e.g. our 2-level namespace) I'd probably have a slight bias toward doing things the ELF way rather than the macho way; most people I've talked to who've worked a lot with macho don't speak too highly of it :)
Nov 8 2017
Nov 7 2017
Yes; in particular, musl's implementation of printf has some code where it promotes everything to long double, so if long double is f128, that causes several of these functions to get linked in to every binary, for no real benefit to the user.
Yeah. Or not; I think you can still use __float128 types in clang, so if you did that then it would use these methods too. And as long as long double is f64 then none of these functions would get pulled into every program that uses printf (which is the case now), so it wouldn't impose any cost on anyone that doesn't actually use f128. So in that case there's not really a reason not to have it.
Also btw I still think I want to back out the "long-double-is-f128" ABI from wasm. But either way it probably doesn't hurt to have these in compiler-rt.
BTW I'm still trying to get caught up, so if there's some review or something you have in flight that you want me to get to first, just let me know.
Oct 27 2017
Oct 24 2017
If you wanted to come up with your own set of syscalls or lower-layer stuff, You'd basically be making up your own OS, which you could name. But libclangrt.builtins in particular doesn't really depend on the OS, it's mostly just architecture-specific or processor-specific stuff. So you could reuse the clang support for that.
D39218 uses the path we want want if the OS is 'unknown'. But shouldn't we make it so that any 'wasm-*-*' triple behaves the same way? i.e. someone who wants to put their own OS name in there but wants to use that same layout (or for that matter, maybe we want to adopt that in emscripten too).
I don't necessarily object to this accessor.
But for our generic non-emscripten toolchain support that we are adding right now, maybe we should make it have whatever defaults we are building now with any OS (not just explicitly "unknown")?
oh wait it's in the subject line... just ignore me :D
Can you add to the commit description that this fixes the build with -DLLVM_LINK_LLVM_DYLIB=ON?
Oct 23 2017
Yeah, this seems OK for now.
I think at some point we may want to revisit the overall goals of the representation that we discussed originally (i.e. not having to encode a symbol table outside of wasm globals, a wasm object file possibly being directly loadable, etc), once we have a prototype and the constraints are clearer. Probably then it will be more clear what to do in cases like this.
Oct 20 2017
LGTM, assuming I am correct in thinking that this will not affect the current wasm-elf assembly output (which does emit .loc directives).
Should symbols actually be allowed to have the same value? How do we handle aliases?
Oct 18 2017
Oct 10 2017
Oct 9 2017
Cool, and the code is even shorter this way too 👍
Oct 6 2017
Oct 5 2017
Removing the requirement works fine now because the passes are set up correctly. The original intention behind getRequiredProperties in this case was that if the pass configuration were to be changed and PEI were to be called before regalloc on a target which wasn't prepared for that, there would be an easy assertion to say what went wrong.
Oct 3 2017
- remove spurious diff
Sep 28 2017
Oh, right, implicit locals. Yeah I don't think it's really worth thinking about too much.
This strategy seems fine to me.
How would this CL be different if it also affected the ELF flavor though? I don't mind fixing s2wasm if it makes LLVM simper.
Sep 27 2017
Missed this yesterday, sorry.
Sep 26 2017
I think it probably makes sense to use the function/global index space for the compiler and linker, and user-oriented tools (e.g. objdump, nm). For things like obj2yaml or readobj, maybe a something more raw would be appropriate, but I'd guess that it's not a super-high priority, other than it's nice to have those for testing.
oh also it looks like you left a couple of words out of the first paragraph of the commit message.
That can be a separate change though.
Is this in tool-conventions yet? If so, should update.