- User Since
- Jan 18 2013, 11:30 AM (463 w, 5 d)
Minor suggestion in the docs, but otherwise LGTM.
Okay, that's fine, I have no objection to removing it. LGTM.
Okay. I guess the only choice, then, would be to not try to parse a template deduction guide when you're in a context where template deduction guides aren't allowed. Maybe that's not actually reasonable.
Trivial relocation doesn't imply that types have to be safe against being suddenly relocated during the middle of operations while they're not in a safe internal state. That is not a consideration.
I am perfectly happy accepting that syllogism. Passing a value in registers is a trivial relocation. If there is a reason your type shouldn't be trivially relocated, you should not make it trivial_abi.
Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.
The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST.
Mon, Dec 6
I agree that you shouldn't call suspend, but doesn't coro.end have the behavior of marking the coroutine done? Should we just be calling coro.end on this path?
I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.
Don't worry about GC::Strong. ObjC GC is thoroughly dead (unless there's GNU-runtime interest in it?), and AFAIK we've never made any effort to incorporate it into our treatment of non-trivial structs.
Sun, Dec 5
That's not an unreasonable idea, and we did consider it, and in fact it's still on the table as something we could do. However, it's a significantly more complex change, and we're not committed to doing it, and so we wanted to at least put this warning in place to help people avoid a relatively common bug.
Thu, Dec 2
Well, I don't envy you the amount of work you're going to have to do to get ObjC working on a Harvard architecture, but this patch LGTM as progress.
This looks great, thanks. Please feel free to commit with the requested minor change to the docs.
I'm sorry for holding this up for so long by just not responding to your pings. Yes, I have no objection to you going forward with this patch, since we're still explicitly saying that it's not yet ABI-stable.
Wed, Dec 1
Tue, Nov 30
Those all look good, thanks.
Mon, Nov 29
Your example is an obvious ODR violation. You've got two totally different classes with external linkage with the same name and namespace. If this somehow isn't blowing up in GCC, I'd be very surprised; it might build, but if these are linked into the same image, the v-table for one of these classes is going to be replaced by the v-table for the other, and you will almost certainly get incorrect dynamic behavior.
Thanks, this is exactly what I was looking for. Just some straightforward style/design requests from here.
I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.
Wed, Nov 24
If you aren't sure what a comment means, please feel free to CC Richard or me, and we might be able to help.
Tue, Nov 23
Your builtin is using custom type-checking (t), which suppresses all the normal conversions that happen on expressions. Specifically, it skips lvalue-to-rvalue conversion, so in this example the argument ends up being an l-value reference to a variable rather than an r-value loaded from that variable. In addition to confusing constant evaluation, it would also make this look like an ODR-use of the variable, which would be subtly wrong in some C++ cases. The fix is to explicitly request the standard l-value conversions, which you can do with code like:
Sun, Nov 21
Platforms are permitted to make stronger guarantees than the C standard requires, and it is totally reasonable for compilers to assume that malloc meets the target platform's documented guarantees. Arguably, libraries should not be replacing malloc if they fail to meet the platform's documented guarantees.
Fri, Nov 19
Thu, Nov 18
We don't properly handle lookup through using declarations when there is a linkage spec in the common chain.
Wed, Nov 17
Mon, Nov 15
If we do need to support constant expressions of this, I think we should have the constant evaluator produce an expression rather than an l-value, the way we do with some other builtin calls. That should stop the comparison problem more generally.
Do any of the proposed use cases actually require this to be a constant expression? Some of these patterns can be cheaply implemented with code generation even if the target doesn't otherwise support constants; for example, we could just mask the THUMB bit off on 32-bit ARM.
Sat, Nov 13
I don't know for certain, but I would guess that the kernel wants to get the address of the first instruction in the function for the purposes of some sort of later PC-based table lookup, which means that yes, it probably *does* need to bypass descriptors on CHERI / Itanium / whatever else.
Fri, Nov 12
Yeah, I think this either needs deployment restriction on Apple platforms or it needs the thunk / weak-linking approach from the original patch when the target OS isn't recent enough. @ldionne should know the right OS versions.
Thu, Nov 11
Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.
Nov 8 2021
Hmm. I wonder if that's related to the problem uncovered by the verifier in https://reviews.llvm.org/D113352.
Nov 7 2021
Nov 6 2021
Seems like a good idea to me. Minor request; otherwise, feel free to commit.
Nov 5 2021
Yeah, I think that's a better name. The documentation can say that ideally this also wouldn't include things like the THUMB bit, but there are technical limitations that make it hard to achieve that.
Nov 4 2021
std::addressof(&someFunction) certainly ought to return a signed pointer under ptrauth, so if your goal here is to get a completely unadorned symbol address, I think you do need a different builtin, and it probably ought to return a void* to emphasize that it shouldn't be used as a normal value. Maybe it should even be semantically restricted to require a constant decl reference as its operand? Related and perhaps illuminating question: if it were implementable, would you also want to force the suppression of lazy-binding thunks and/or decorations like the THUMB bit?
Nov 3 2021
Nov 2 2021
Mostly LGTM, although I am not the most unbiased reviewer. :)
Nov 1 2021
Thanks. @hubert.reinterpretcast, @qiucf, can you verify that other compilers for PPC follow the logic for TF / TC that we now have with Elizabeth's patch? There are three different type specifiers (long double, __ibm128, and float128_t) which represent formally distinct types, and IIUC there are a bunch of different flags and target options that change the meaning of long double and/or disable/enable __ibm128 and float128_t. We care about exactly which type is produced by the mode attribute, so you'll have to do something which is sensitive to the exact formal type, like _Generic or a C++ template or doing a pointer conversion without a cast; checking code generation will only tell us the underlying format.
For posterity in case someone tracks down this review: TC corresponds to an unspecified 128-bit format, which on some targets is a double-double format (like __ibm128_t) and on others is float128_t. The bug in the previous patch is that long double is only safe to use when it's known to be one of those formats.
Oct 30 2021
Well, we've been using ptrauth as a keyword/prefix/etc. to write software at Apple for more than four years, and we have tens of thousands of lines of code using that scattered across several dozen projects, which is probably at least half of the pointer authentication code in the world. I did specifically reach out to the ARM ELF group in the early days of that effort asking that they standardize on ptrauth instead of introducing yet another abbreviation for the extension, but they'd already made a file with "pauth" in the name, so now for better or worse I think we're stuck with having yet another abbreviation for the extension.
I think Peter is saying that if you can make Clang emit correct ranges for v-tables — specifically so that the ranges don't cover the type_info pointer, which we don't emit enough metadata to safely remove — then you can also go ahead and remove the special case where we only eliminate loads of function pointers. That's assuming we don't need to support old bitcode.
Oct 29 2021
Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?
Oct 26 2021
Hmm. Generally these cases are expected to handle the situation where there's no result slot passed in, which currently isn't exclusive to an ignored result (although you could argue it should be). IIRC we usually don't pass in a slot when evaluating an expression of agg type as anything except an initializer, e.g. when evaluating the E in E.member. The general fix is to call EnsureDest before accessing Dest. The only reason we wouldn't need to do that is if ConstantExpr is restricted in where it can appear such that that's impossible.
This seems reasonable. You could also give ExitFunctionBodyRAII an explicit pop() operation, but either way is fine.
Oct 23 2021
Minor grammar nit with a comment, but otherwise LGTM.
Oct 22 2021
Oct 20 2021
Well, I think the basic problem is that unlowered coroutines cannot be modeled as normal functions very well in LLVM IR, and the coro intrinsics are doing their best to muddle through given the unstated requirement to not bother anyone else too much. We have major representational problems with modeling some of the things we need, especially the distinctions between ordinary-ish behavior in the ramp function, logically separate from the coroutine, vs. things that are semantically part of the coroutine body.
Oct 19 2021
@rjmccall, please correct me where I'm wrong :)
The standard answer is that compilers are designed to work with a specific set of system headers. In the Clang-on-Windows case, that's complicated by the fact that many people acquire Clang separately from the rest of the build environment (although Microsoft does distribute Clang officially now, right?), but I think the standard answer is still ultimately the correct one: Clang is designed to support the MSVC system headers that are currently out in the world, and whenever Microsoft releases new system headers, it's possible that you will need a new version of Clang.
Oct 18 2021
Oct 15 2021
I don't think we currently hold very strongly to that ActOn vs. Build philosophy across all productions, but it does seem like a good idea.
Oct 12 2021
Yes, I think it would be fine to fork off a conversation about what the ABI should be; I didn't meant to completely derail this patch. To be frank, my expectation was that more of this was already settled and documented, so we just needed to add that documentation and clean up a few details.
Ok. I'm not an expert in this code, but it seems fine to me.
Okay. Thanks for the explanation, I think that was helpful.
Oct 11 2021
I did skim the thread, and it seemed to me that Peter's objection was largely to the reuse of !type, which he feels wasn't meant to carry these semantics. Peter leapt to the idea of a new constant instead of a more general metadata, but I don't understand what that leap is meant to achieve, which is why I'm engaging with the thread.
I don't entirely understand the purpose of vfe_eligible. It sounds like it expresses that it's okay for this value to be replaced if we can prove that nothing ever uses it. Surely that's universally true, albeit usually extremely hard to prove. So maybe the right general approach is that we should be saying something about the v-table object: that we know certain fields in it are only accessed in specially blessed patterns, and then this !type metadata or whatever lets us prove that certain patterns don't exist. I don't know that a new constant is the best way to express that, as opposed to some sort of metadata on the v-table. At the very least, maybe that gives us some direction towards a more general name for the constant.
Okay, thanks, Alexey. LGTM, then.
This mostly looks good, but I'd like Alexey Bataev to sign off, since he authored the OpenMP support.
Oct 8 2021
Thanks. This LGTM when all the patches it depends on are checked in.
I agree. It doesn't have to be a CodeGen test, just anything that directly verifies that we get the right type, since I think those calls can succeed due to promotion.
Patch LGTM, but please remove the test, it's been folded into a test I added in my patch.
Hmm. I decided the ObjC case needed a more complex change and went ahead and committed that fix here: https://github.com/llvm/llvm-project/commit/5ab6ee75994d. That obviates the need for the new test; sorry for the runaround. Please rebase and I'll review.
Oct 7 2021
Okay, so does this mean that this works on arbitrary targets now? If so, should Clang be using it unconditionally?
Can you just do one patch for all the OpenMP changes, and then put the final removal in a separate patch?
We should take the opportunity to add a test here for the ObjC case. It should be testable with something like this, where you should see the store inside of the loop instead of once in the prologue: