User Details
- User Since
- Jan 18 2013, 11:30 AM (492 w, 5 d)
Yesterday
Tue, Jun 28
Looks good, thanks! Approved with the rename as discussed.
Mon, Jun 27
Is there no way in LLVM to load partially-initialized memory and then freeze it that will preserve the parts of the value that are initialized? Because that seems like something that LLVM needs to be providing.
Sun, Jun 26
Thank you, LGTM.
Sat, Jun 25
Fri, Jun 24
LGTM
Seems right to me.
Fri, Jun 17
Okay, LGTM then
Thu, Jun 16
Mon, Jun 13
Yeah, I think the appropriate thing is to just treat the entire asm statement like it's a single full-expression. As always, that implies an annoying lifetime for blocks and C compound literals.
Minor suggestion for a cleanup, then LGTM.
LGTM. Don't worry about Swift, we'll handle it.
Sat, Jun 11
The cleanups arise from expressions in the constraints, right? There are a number of places where ExprWithCleanups does not mean the cleanups are independently nested within that expression; it's notably not how it works ever for something as basic as a variable initializer. If we want the lifetime of temporaries in asm operands to include the full duration of the asm statement, I think we can just declare by fiat that that's how it works for AsmStmt instead of unnaturally wrapping things this way.
Should we just diagnose this and report an error that catching pointers by non-const reference is unsupported on Itanium? We don't support it properly for class pointers anyway.
Fri, Jun 10
Thu, Jun 9
Yeah, that makes sense. Thanks for following that up.
Okay, LGTM
Wed, Jun 8
This seems like the right idea to me, yeah. It doesn't look like the patch handles volatile _Atomic correctly, though.
Tue, Jun 7
Mon, Jun 6
Fri, Jun 3
I completely lost track of this, but if you'd like to rebase it, I can try to make time.
I disagree with the assessment that _Atomic behaves exactly like a qualifier. It *should* (IMHO), but it doesn't. C allows the size and alignment of an atomic type be *different* from its unqualified version, to allow for embedded locks and such. Because the size and alignment can be different, to my mind, _Atomic int and int are different types in the same way as _Complex float and float -- the object representations can (must, in the case of _Complex) be different, so they're different types. The same is not true for const, volatile, or restrict qualifiers -- those are required to have the same size and alignment as the unqualified type.
Are you now arguing that C was wrong to drop qualifiers in return types and so we shouldn't implement that rule, or...? Because your argument is much broader than just _Atomic.
Thu, Jun 2
Okay, I understand. So, first off, I wouldn't really call that a "weak" symbol rather than, say, a lazily-emitted symbol; "weak" already has plenty of different senses, and we should try to avoid coining more. Also, your patch description makes it sound like there's a general bug you're fixing rather than specifically an issue with re-using a single CodeGenerator to generate a succession of modules; please remember that people reading your commit messages don't necessarily know you or contextualize your patches by knowing that you work on Cling.
LGTM
Ah, true, the standard doesn't describe it as a normal qualifier. From the DR, it sounds like the committee certainly expected that this reasoning would also apply to _Atomic, even if that's not quite what they've written, but that the DR submitter seems to not want that behavior. Have you been able to track down the solicited paper mentioned in that DR? In the absence of further indication, I think dropping _Atomic like anything else is the right thing to do; like the other qualifiers, it isn't meaningful for r-values.
Ah, yeah, that seems unrelated (and surprising).
You haven't really described the issue you're having. The code looks like it's maintaining a map from symbol names to the GlobalDecl from which it'll be emitted; can you explain what Cling wants this for?
Tue, May 31
May 19 2022
Thanks, LGTM.
May 18 2022
We definitely don't want ObjC to differ here; thanks for the CC.
May 16 2022
I'm fine with alternatives on the names. I just don't think we want names that imply that the caller and callee are parallel here, as if there were some sort of requirement that the callee always extends. In those conventions, the callee gets passed 8 or 16 meaningful bits; if it wants an extended value, it's responsible for doing that extension, just like it would be responsible for doing the opposite extension if it wanted that.
May 15 2022
On the one hand, I'm not sure 8M statements in a block — or 1M, for that matter — is an unreasonable implementation limit. On the other hand, this patch looks like it only changes overall memory use on 32-bit hosts, because CompoundStmt has a 32-bit field prior to its pointer-aligned trailing storage. Can you check how memory use actually changes? Ideally you'd be able to test it on a 32-bit host, but if you don't have one, just knowing the raw count of allocated CompoundStmt objects for a few large compilation benchmarks would be nice — the ideal stress test is probably something in C++ that instantiates a lot of function templates.
May 9 2022
LGTM
May 5 2022
I think you're right that I signed off on it; I don't remember when or where that was, or what I said about it then, but regardless I can tell you that there are two basic reasons:
- First, the result of demangling to a string is not source code. It produces valid source code as much as it can, and that's a good choice, but there are common cases where it cannot just produce source, and there is no use case that requires it to produce source. Ultimately, this is just a string that is going to be presented to a human.
- Since we're just going to present it to a human, what mainly matters is what the human's expectations are. Approximately no C++ programmers will be confused to see >> terminating a template argument list, in part because it's been a long since since C++11 was available, but also simply because it was unnatural for programmers to remember to include the space in the first place, to the point that any reasonable compiler had to catch it and recover as a common error.
May 2 2022
It should probably be the ABI alignment in all cases; I think the preferred alignment is just supposed to be used to opportunistically over-align things like e.g. local variables, which doesn't seem relevant for the ABI code involving a call to operator new.
Apr 30 2022
IIRC, the reason it works it that way is that "warnings which default to an error" are really "errors which you can explicitly downgrade to a warning". Maybe those ought to be different categories, or maybe we ought to just be telling people to downgrade this specific diagnostic instead of generally using -Wno-error.
Apr 28 2022
My only comment about the design is that maybe it should be __builtin_kcfi_call_unchecked, which does not seem like something you need to hold up LKML discussion for.
Apr 27 2022
Apr 26 2022
Hmm. Allowing a version on -stdlib is intuitively appealing, but I'm not sure it actually gives us the information we need. As I recall, -stdlib selects the high-level stdlib and not the low-level one, and those are related in code but not necessarily at runtime; for example, you can (or at least could, historically) use libstdc++ on macOS, but the underlying low-level stdlib is going to be libc++abi, not libsupc++. And the low-level runtime is the one that actually provides global operator new functions. Is there a way to bridge that gap?
Apr 25 2022
Ideally, I think, we would set this up to work something like ObjCRuntime, where we're making queries to a common place that contains all the information necessary to decide what runtime features are available. In particular, we shouldn't treat Apple platforms as forever unique in providing a stable runtime interface with availability gating.
Apr 21 2022
Sounds like you need an RFC thread on the forums describing the underlying WebAssembly feature and your proposed language design for exposing it in C.
Apr 19 2022
If people think that we'll reliably get the right behavior by the frontend adding sext and zext to call sites but not to function arguments, I can walk you through how to make that change in the frontend.
Apr 18 2022
From Michael Matz's comments on the GCC bug and some of the linked conversation, it sounds pretty clear that the psABI does not require extension. They could certainly be more explicit about that in the psABI, but they're under no real obligation to: the baseline assumption should always be that things not stated are not guaranteed. If the psABI intended extension to be required, the document would really need to state it positively, rather than leaving it to be inferred. Since it does not, extension is not guaranteed by the psABI.
LGTM
Apr 16 2022
LGTM. Could you add a test case?
Apr 15 2022
Sorry, that was a bit of a jumble of different ideas that I pressed into the form of a single message. Let me try to do better.
readnone etc. surely encompass the entire behavior of the call, which of course includes any callbacks it might make. And synchronization needs to be understood as effectively a store, since synchronizing with another thread which has done a store guarantees the visibility of that store on this thread; and thus readonly etc. must imply nosync. So whatever nocallback means, it must be finer-grained, suitable for situations where the stronger attributes cannot be used.
Apr 14 2022
The GCC documentation really ought to specify what alignment assumptions these functions make, if any. But they control the specification here, so if they're making alignment assumptions, then I agree we probably ought to make the same assumptions.
Apr 9 2022
I'm totally fine with a specifier that removes all qualifiers; I just think we can't use it for std::remove_cv.
Apr 6 2022
Apr 5 2022
Apr 1 2022
Mar 31 2022
Unfortunately, this initialization case does allow a situation where
LGTM
Mar 30 2022
Yeah. I think you could probably get away with just doing to the FramePtr argument, but it's probably safer to do it unconditionally.
Mar 29 2022
It seems very likely to me that you could contrive a situation under C++ coroutines where the frame pointer is a parameter to the ramp function by the time you run coro splitting, at least under optimization. For example, https://godbolt.org/z/cGnjTbzrY, which is a reasonable implementation if you need to guarantee non-allocation and intend to assert that the size fits in the buffer. This isn't an oddity of retcon lowering, it's a general problem that emerges from the special way that the frame pointer value is assumed to be live throughout the function, which requires it to be protected against this undef'ing of arguments which would otherwise be reasonable.
Does that actually fix anything? The problem is that the use-list of the frame pointer is being destroyed in the clones if it happens to be an argument of the ramp function. We have to stop that from happening. I don't see a solution to that other than either changing VMap or introducing an intermediate value so that it isn't just a use of an argument, and the former seems much easier.
Hmm. Doing this after doing the clones means you're only actually doing the replacement in the ramp function, which I think means it'll explode in the clones if there are uses remaining. On some level, that might be okay, because I don't think the Swift frontend ever actually uses the result of coro.begin in retcon functions except to pass it to coro.end, and retcon lowering of coro.end just ignores the argument. On the other hand, we shouldn't leave something around that'll blow up if the code pattern ever changes.
Mar 28 2022
Hmm. We know that the big picture here, distinguishing pointers by pointee type, is going to be disruptive and will probably need a specific enabling/disabling option. I'm not sure that distinguishing only by pointer depth is a minor enough step that it shouldn't be treated the same way; in particular, it's going to start treating void * as incompatible with e.g. char **.
Mar 25 2022
I think C++11 is pervasive enough that removing the space is fine. Demangler output is not necessarily valid source anyway.
Mar 24 2022
Mar 2 2022
LGTM
Feb 28 2022
This is one of those patches that's difficult to review because it's so hard to foresee the consequences of changing the concepts. That said, I think the basic idea here seems reasonable.
Feb 25 2022
Is there something which stops you from taking the address of a kernel and then calling it? If not, are there actually any uses of kernels in the module that shouldn't be rewritten as uses of the clone?
Feb 17 2022
It makes sense to me to do this on top of __is_trivially_relocatable, and if that returns true for more types in the future, wonderful. Obviously, it needs to be used in non-ABI-impacting ways. I'm not the right person to review this patch, though.
Feb 12 2022
It's nothing to do with your patch, and you don't need to worry about it.
Feb 11 2022
And this should be raised as an Itanium issue.
Changing the C++03 POD definition is going to be substantially ABI-breaking at this point. Any logic to change it needs to be platform-specific.
Feb 10 2022
Sure, LGTM
Feb 8 2022
If we want to assume a weaker alignment, we should change the calculation of allocationAlign rather than just changing this point downstream of that computation. But replacements of the standard operator new are restricted and cannot simply choose to return less-aligned memory; at the very least, the test program is not portable.