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.

As is, I think this conflicts with -ffreestanding assumptions or at the very least the spirit.

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.

LGTM

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.

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.

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...

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.

I'm ambivalent about this change. To me, it falls into the category of "stop passing random ld options to the compiler driver".

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?

I agree that I don't think it generally helps all that much. Under memory pressure it can even result in additional writes.

Last update introduced a lot of unrelated changes? But the actual intended change seems fine now.

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.

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.

It should be noted that xstore is currently misassembled compared to GNU as.

In D111863#3072023, @housel wrote:

It's also worth noting that FreeBSD's version of libgcc exception handling is actually based on the libunwind code, with a local patch that implements compatibility with libgcc __register_frame by changing it to parse an entire .eh_frame section (in a slightly more ad hoc fashion than this code). Having this new entry point in-tree would simplify the FreeBSD local changes.

In D111863#3071651, @lhames wrote:

In D111863#3071353, @joerg wrote:

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.

I don't follow this. Does libunwind provide some way to register FDEs without parsing the FDE content? If so we can definitely use that, but we should still process the whole section: ORC links objects (not functions), and we should register every FDE for an object when it's linked in.

It's also worth noting that ORC and MCJIT have always called __register_frame on every frame, which seems like it should be at least as much work as this.

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.

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.

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?

Why are all the changes from separator character to separator string necessary or desirable?

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.

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.

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?

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.

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.

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...

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?

In D106577#2899715, @aaron.ballman wrote:

In D106577#2899711, @joerg wrote:

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.

How does the value of a macro get impacted by a runtime locale?

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.

Is there any reason for breaking C++03 code here? I don't see the advantage to that at all.

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.

Please make sure at the very least that it doesn't get matched if a function named strlen and also test for that.

We inherited the lack of int128_t support on 32bit platforms from GCC. IMO we should fix that, the only tricky one is division.

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).

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.

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?

"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?

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.

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.

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.

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.

Resummited

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?

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.

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++.

The NetBSD part is most definitely not acceptable.

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.

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.

off_t is s signed type. Please fix the description.

Do we really want to change the code here? It is perfectly well defined behavior.

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.

"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.

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).

You can do an anon-mapping with size+pagesize in that case and map over it with MAP_FIXED.