User Details
- User Since
- Nov 23 2012, 10:16 AM (539 w, 4 d)
Apr 11 2022
The patch contains at least one user visible change that would be quite surprising: it is no longer possible to intentionally set a break point on std::move.
Apr 9 2022
As is, I think this conflicts with -ffreestanding assumptions or at the very least the spirit.
Mar 29 2022
Why is this fold preferable to (X & (X-1)) == 0? At least on all architectures without native population count, the binary-and based test is preferable and it might even be better with it.
Mar 13 2022
LGTM
Mar 12 2022
I would find it quite surprising to find two binaries produced with the same command line now resulting in different build-ids, if that is the only difference. Especially when the cryptographic hash function is explicitly set.
Explicit check for Z_MEM_ERROR and assert that it is otherwise Z_OK. Other return values would point to a programming error in zlib.
But that doesn't really matter much as long as it is uninitialized virtual memory? This is almost always used for a very short time as well.
compressBound gives the maximum possible size for the output, so of course it can be larger than the input. In combination with resize_for_overwrite, that should be cheaper than possible multiple copies for the loop if there is even a slight chance that the data doesn't compress that well. The code complexity is certainly much higher. The only expected error from compress2 here is Z_MEM_ERROR when it can't allocate the dictionary etc. As such, we could convert that one into a fatal error easily.
Feb 17 2022
I'm ambivalent about wether to use sync_* or atomic_*, but last time I looked at this, we generated the generic unsized variant a lot, even when the arguments have a known static size.
Feb 15 2022
Wouldn't the correct solution for Illumos be to use posix_madvise instead? Possibly with __EXTENSIONS__. Declaring prototypes like this is just begging for problems long term...
Feb 14 2022
Well, it doesn't work with GCC either, that's why I don't care much about this change. It just attempts to legalize a user bug (using a linker option directly as a compiler flag). But I don't care enough to object either.
Feb 13 2022
I'm ambivalent about this change. To me, it falls into the category of "stop passing random ld options to the compiler driver".
Jan 24 2022
Jan 19 2022
Correct me if I'm wrong, but I don't think this stubs are async signal safe nor will they work for preemptive multitasking systems?
Jan 9 2022
Dec 17 2021
I agree that I don't think it generally helps all that much. Under memory pressure it can even result in additional writes.
Dec 12 2021
Last update introduced a lot of unrelated changes? But the actual intended change seems fine now.
Dec 11 2021
Dec 9 2021
Nov 16 2021
Exactly. This patch only provides a way to tick a checkbox on a stupid compliance list. SHA2 provides no security benefit here for the price of slower linking. If we want to use a more modern cryptographic hash function, the primary criterion should be performance, IMO.
Nov 4 2021
I fully agree that constructing DT can be expensive especially if it is potentially done multiple times. I don't mind using it when available, but the gain seems limited for forcing it.
Oct 25 2021
Oct 22 2021
It should be noted that xstore is currently misassembled compared to GNU as.
Oct 19 2021
Oct 18 2021
I would strongly prefer if ORCv2 doesn't depend on this. It essentially forces libunwind to parse the whole section just to find the delimiters of the FDEs. That's a lot of unnecessary work as JIT use normally allows registering functions individually.
Oct 12 2021
Did this get an audit for existing users? It can be argued in both ways. E.g. "".isdigit() in Python is False, because there is no digit contained and that's pretty much the same use case.
Oct 6 2021
Can't you wrap iterator_range<T> and possibly even support Twines like that? That is, don't extend the life time of the iterators, but store it in the range?
Sep 28 2021
Why are all the changes from separator character to separator string necessary or desirable?
Aug 10 2021
LGTM. Maybe include a small hint in the commit message that xlC never shipped cross-compiling support and the difference is therefore not observable anyway.
Aug 9 2021
clang is fundamentally a cross-compiler only. I don't see any point for having host-specific branches in this case at all. Either the macro should be specified for the target all the time or not at all, but it should most definitely not depend on the host. That's actually breaking a number of existing use case for clang in subtle ways, e.g. partially preprocessed files (-frewrite-includes) should behave the same on any platform given the same command line.
Aug 6 2021
I'm puzzled by this change. I don't think we have any case so far where the compiler behavior changes with the host OS and I don't think it should. What's the point / use case of this macro?
Aug 4 2021
This discussion is becoming pointless. Clang is not a 1:1 replacement for later GCC versions. Easiest example is the support for __builtin_apply and friends.
Aug 3 2021
Yes, there are quite a number of small differences in builtin support and even certain macros that just invite even more trouble than this. This is IMO begging for hard to trace down errors.
Aug 2 2021
What do you mean with "GCC macros are correct"? Clang is *not* GCC 5 and not 1:1 compatible with GCC 5. Project that have checks like that should arrive in 2010...
Jul 26 2021
I still don't fully understand the original comment from Joerg. The encoding of L'a' cannot change at runtime; it's a literal whose encoding is decided entirely at compile time. @joerg -- did you mean that Clang produces a different literal encoding depending on the environment the host compiler is running in?
Jul 23 2021
This patch is certainly wrong for NetBSD as the wchar_t encoding is up to the specific locale charset and *not* UCS-2 or UCS-4 for certain legacy encodings like the various shift encodings in East Asia.
Jun 30 2021
Is there any reason for breaking C++03 code here? I don't see the advantage to that at all.
Jun 21 2021
Thanks, that's better. The clang front-end option is not directly relevant for the semantic of the intrinsic, so it would be better to explicitly specify it was not being fuseable unless the operand degenerates into a single operand. Otherwise the specification sounds reasonable.
Jun 13 2021
Please make sure at the very least that it doesn't get matched if a function named strlen and also test for that.
Jun 8 2021
We inherited the lack of int128_t support on 32bit platforms from GCC. IMO we should fix that, the only tricky one is division.
Jun 4 2021
May 26 2021
May 23 2021
May 20 2021
May 12 2021
One of the mots important use cases of __builtin_constant_p is ensuring that inline assembler variants that require immediate arguments are DCE'ed. There are some other use cases where it is used for compile-time assertions of invariants, so we do want to be as correct as possible. All that said, I fully agree that is.constant(undef) should fold to false as it is consistent with the immarg parameter attribute and other places. Even stronger, I would argue that it applies even to is.constant(freeze undef).
Apr 23 2021
There are two approaches that work for reviewing: starting from clang and going down or starting from compiler-rt and going up the layers. I'd prefer to do the latter as it means meaningful testing can be done easier. There are two natural steps here: clang flag+IR generation is one, llvm codegen and compiler-rt is the other. The clang step should include tests that ensure that proper IR is generated for the different flag combination (including documentation for the IR changes). The LLVM step should ensure that the different attributes (either function or module scope) are correctly lowered in the instrumentation pass and unit tests on the compiler-rt side to do functional testing. The unit testing might also be done as a third step if it goes the full way from C to binary code.
Apr 22 2021
This review doesn't make sense to me. Why add a flag that isn't affecting anything? Why add a flag that isn't even tested?
Apr 21 2021
"Keeping the original spelling around" would assume that the input is not using a stateful encoding. That seems worse as assumption than giving the canonical output in UTF-8 and shifting the problem to the user's editor?
Apr 7 2021
This sounds wrong. If you are using 'x86_64-freebsd' as triple and -m32, it should still call 'x86_64-freebsd-ld', but it is the responsibility of the driver to ensure that also the right set of linker flags are passed as well. Compare netbsd::Linker::ConstructJob for one way to handle this.
Mar 31 2021
Mar 30 2021
Yeah, I hit a compilation error somewhere depending on the include error. I forgot the details, this has been sitting around in my tree for while.
Mar 29 2021
Mar 28 2021
The NetBSD part looks ok, but also lacks proper testing. I'm not sure anyone but Linux cares at all otherwise as they lack 32bit SPARC support.
Mar 10 2021
Mar 8 2021
Mar 5 2021
That covers my case. I'm not completely happy with the need to rethrow, but the alternatives are much worse. It's a bit sad that we can't optimize it away, too.
Feb 18 2021
Feb 17 2021
Registers_arm64::jumpto in UnwindRegistersRestore.S.
Let me rephrase then. The restore code explicitly ends in "ret x30". It cannot set "pc" separate from "x30" in the current form. I'm not saying that the DWARF register shouldn't be supported, but this change doesn't do that.
I don't get this change. The restore code is literally returning by jumping to x30, so how is splitting PC and LR supposed to work?
Feb 8 2021
The test case needs to be resorted, so that the DOTEXPR test comes last, but otherwise this change works for me.
I have one concern about sparc-relocations.s, but that is not specific to the change at hand. IMO we should also have a variant of all the relocations with a fixed values of sym to test that the right bits are actually set. That would also make comparing the fixup handling easier e.g. to GNU as.
Feb 5 2021
I can't speak for other systems, but forcing libpthread to be linked is in general not desirable as it creates a non-trivial runtime cost, especially when <thread> is not used. That's still a pretty common use case of C++.
Feb 4 2021
The NetBSD part is most definitely not acceptable.
Jan 30 2021
First of all, I find this patch to be nearly impossible to read. It seems to mix a lot of refactoring with a functional change, making it very hard to focus on the core.
Jan 15 2021
It might be specific to the jump table case, but it should be instructional on how to do it. One important point is that it avoids inter-section relocations, which are a problem at least on MIPS.
Some targets already do this. Please check that you don't create regressions, especially on PowerPC.
Nov 30 2020
off_t is s signed type. Please fix the description.
Nov 23 2020
Do we really want to change the code here? It is perfectly well defined behavior.
Nov 19 2020
The difference is whether we promise to be instruction precise or not. I'm not sure we do or want to promise that as default.
I have no problem with the change, but please adjust the description to take about -funwind-tables. I don't think we do async by default, at least not by design.
Oct 29 2020
"It compiles with GCC" is not really helpful as argument because the observed behavior depends on a lot of internals. It is quite frankly impossible to be 100% identical to GCC's behavior. What LLVM guarantees is:
(1) Folding to false happens as late as reasonable possible. It happens most importantly after the first round of function and loop optimizations.
(2) Folding to true happens as part of constant folding etc as early as the condition is evaluated.
(3) The result is propagated to derived expression and those are folded recursively, including dead code elimination. This happens even for -O0.
Oct 28 2020
I completely agree with Roman that this patch is very undesirable since it has a high chance of breaking other reasonable uses of the intrinsic. In fact, I would say that the observed behavior here is perfectly reasonable under the definition of @lllvm.is.constant. The original code for LLVM at least should be just __builtin_constant_p(b) && b > -129 && b < 128).
Oct 27 2020
You can do an anon-mapping with size+pagesize in that case and map over it with MAP_FIXED.