In D67847#1693694, @rnk wrote:

In D67847#1691898, @jyknight wrote:

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

It's easy to crash LLVM even without this change, so anyone running LLVM better have core dumps configured the way they like. Failed asserts raise SIGABRT already, for example, and we have tons of those. The only difference is that now end users, who may have never configured this stuff, may see more crashes. If it's a problem, I'd consider it QoI: we should fix the report_fatal_error to use proper diagnostics anyway so end users don't see them as often, just as we would treat any other user-visible crash.

Since this change is not android-only, please update change description, and update some non-android tests.

In D64830#1813723, @codehippo wrote:

@andreisfr @jyknight @ivanbaev Ping? Are there any news concerning these pull requests?

Reopening, since this didn't actually land yet. BTW, this review still has the wrong contents in the latest uploaded-diff (showing llvm changes, not clang changes).

Can you add a comment noting the deficiencies of these conversions?

I think you pushed the incorrect change? This was the clang half, but now it's showing the LLVM half. Can you re-upload the diff for the clang half?

In D72197#1806495, @dblaikie wrote:

I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))

LGTM modulo some minor wording.

In D72197#1805825, @jyknight wrote:

I don't see the behavior you say gnu as has.

I don't see the behavior you say gnu as has.

BTW, the commit here should mark llvm.org/PR44324 as fixed by it.

In D71037#1789574, @Tyker wrote:

after fixing the bug rsmith spotted. there is no test failures in clang + clang-tool + clangd with UseTabStopOption = true (i found this quite surprising but i double checked). the previous test result were incorrect due to this bug sorry. however I am still not sure that UseTabStopOption = true is the right default for every uses of getSpellingColumnNumber.

In D70157#1788418, @reames wrote:

In D70157#1788025, @jyknight wrote:

.push_align_branch_boundary [N,] [instruction,]*

I'd like to raise again the possibility of using a more general region directive to denote "It is allowable to add prefixes/nops before instructions in this region if the assembler wants to", as I'd started discussing in https://reviews.llvm.org/D71238#1786885 (but let's move the discussion here).

James, I think this proposal is increasing the scope of this proposal too much. It also ignores some of the use cases identified and described in the writeup (i.e. the scoped semantics). I'm open to discussing such a feature more generally, but I'd prefer to see a more narrowly focused feature immediately.

I'd like to raise again the possibility of using a more general region directive to denote "It is allowable to add prefixes/nops before instructions in this region if the assembler wants to", as I'd started discussing in https://reviews.llvm.org/D71238#1786885 (but let's move the discussion here).

I do like the idea of generalizing bundle_lock to mean generally "keep these instructions together and don't introduce anything extraneous in the middle". So, ".boundary_align <argument>" would apply to the next instruction-or-instruction-bundle, and will emit nops at its location, such that the next instruction is guaranteed to have the proper "within but not ending at a given 2**<arg> block" alignment.

Yes, I had expressed a dislike to adding these alises, as there's no pressing need to do so.

Honestly, I both strongly agree with this, and see the argument as a somewhat lost cause. I think in practical terms, we *will* have to support something which works for both compiler generated assembly and legacy hand written assembly. I dislike that fact and the design it encourages, but it's reality.

In D71238#1776518, @reames wrote:

In D71238#1776512, @skan wrote:

In function X86AsmBackend::alignBranchesBegin, I added the comment

// The prefix or nop isn't inserted if the previous item is hard code, which
// may be used to hardcode an instruction, since there is no clear instruction
// boundary.

I'm sorry if I didn't make it clear. As far as i am concerned, I think the hard code case should be dealt with.

I'm still not following. Can you explain the use case here? What test case differs based on the presence of the "hard code" support in the original patch?

In D70157#1771832, @reames wrote:

I've been digging through the code for this for the last day or so. This is a new area for me, so it's possible I'm off base, but I have some concerns about the current design.

First, there appears to already be support for instruction bundling and alignment in the assembler today. I stumbled across the .bundle_align_mode, .bundle_start, and .bundle_end mechanism (https://lists.llvm.org/pipermail/llvm-dev/2012-December/056723.html) which seems to *heavily* overlap with this proposal. I suspect that the compiler support suggested by James and myself earlier in this thread could be implemented on to this existing mechanism.

Second, the new callbacks and infrastructure added appear to overlap heavily w/the MCCodePadding infrastructure. (Which, admitted, appears unused and untested.)

I have some other concerns about the code itself, but after pondering this a little bit, I'd like to instead bring up some rather more general concerns about the overall approach used here -- with some suggestions. (As these comments are not really comments on the _code_, but on the overall strategy, they also apply to the gnu binutils patch for this same feature.)

Thanks for the comments, they help a little. But it's still somewhat confusing, so let me write down what seems to be happening:

In D70508#1755232, @aprantl wrote:

In D70508#1754479, @jyknight wrote:

The test dwarf-size-field-overflow.test now takes over 60 seconds to run, which seems quite excessive.

Is there no way to test this in a less-expensive manner?

I almost halved the number of iterations in 1b9ef. On my iMac Pro in Release+Asserts it's running in 1.18s wall clock time now. Note however that even before my change it was only taking ~5s. Were you measuring a debug build?

The test dwarf-size-field-overflow.test now takes over 60 seconds to run, which seems quite excessive.

Overall comment: this whole change needs more comments, everywhere. Both for the added functions, and for the test cases. There is almost no description of what's happening, and it could really use it.

In D70157#1747551, @davezarzycki wrote:

In D70157#1747510, @xbolva00 wrote:

Even though core2 isn't affected by the erratum, core2 code can run on CPUs that do have the bug (and core2 is a popular target for code that needs to run "everywhere"), therefore all target CPUs that predate a hardware fix really

So perf of all code/generic codegen will go down. This is not very acceptable. -1 for making this enabled by default for generic codegen.

I'm okay with this not being the default if we document somewhere that LLVM assumes that machines have up to date microcode installed.

Making this work only on ".if" is IMO a non-starter, at least without understanding why. So I looked into what broke with llvm/test/MC/MachO/reloc-diff.s.

Update for comment.

Ping!

Also, since this means we are no longer simply implementing according to GCC's documentation, I think this means we'll need a brand new section in the Clang docs for its inline-asm support.

I think -Wuninitialized (UninitializedValues.cpp) should be taught how to detect the use of output variables in error blocks, at least for trivial cases.

In D69751#1737360, @xbolva00 wrote:

It should never be required in normal circumstances, but this function can also be used while debugging, where that would be convenient.

Not sure how it can be used currently, maybe with a big luck, since there is null pointer dereferencing.

I am gonna commit this.

In D64830#1736412, @andreisfr wrote:

@jyknight , as I'm understood, Cadence doesn;t want to publish Xtensa ISA. But it seems that documentation from the link that you provided is quite actual, probably it was published when Xtensa was owned by Tensilica. Also, the code can be reviewed by people from companies that use Xtensa IP in their chips.

Oops, it was my fault that I put an access before the check.

@andreisfr : Ping?

Updated release notes in d11a9018b773c0359934a7989d886b02468112e4.

Updated release notes in d11a9018b773c0359934a7989d886b02468112e4.

In D67983#1723681, @rjmccall wrote:

We could probably do a quick check to see if the class informally conforms to the protocol. +copyWithZone seems to be marked unavailable in ARC; not sure if that would cause problems for such a check.

In D67983#1723376, @thakis wrote:

After this, Class can no longer be used as a key type in an Obj-C dictionary literal. Is that intentional?

In D64830#1634592, @aykevl wrote:

I do not think there is a reason why the Xtensa architecture shouldn't be included in LLVM. As a comparison, there is also the proprietary (Google) Lanai architecture which was added at a time when there was little public information on it and to this time nobody can actually buy Lanai hardware (there is only a simulator). As a comparison, Xtensa has easy to get hardware (in the form of the ESP8266/ESP32) and is already supported in GCC/binutils.

This is bogus -- the RISCV GCC implementation is broken. It should not be mixing libatomic function calls with direct atomics.

In D67867#1698273, @abrachet wrote:

In D67867#1696908, @MaskRay wrote:

The commit was done in a hurry. For the initial commit of a brand new project that sets up the project hierarchy, this seems to have received fewer than enough thumbs up. Many points raised in the review process were just shrugged off.

I don't know if it matters anymore because this was committed but I agree with @MaskRay. His suggestion of using llvm-objcopy to rename the symbols for tests makes much more sense to me. I haven't seen a libc that does testing in an ergonomic way and this suggestion seems the best to me, frankly.

There is a lot going on here, it's hard to follow it all in one patch, and I think some comments got lost because of this. I feel like a lot of big design decisions were made here, did I miss something on the libc-dev mailing list?

The close was due to phabricator problem, reopening.

In D67867#1692075, @sivachandra wrote:

In D67867#1691962, @jyknight wrote:

The issue really only exists when you refer to object files across standards layers -- e.g., using an object file that exposes the POSIX symbol "open" from an object file implementing ISO C. If you make sure to always strictly-layer the libc, so that ISO C-implementing object files don't use any POSIX-exporting object files, and so on, you won't need to mark anything weak. For the example of "open", you'd have an internal implementation of open in its own file, only exposing libc-internal symbols. Then fopen (ISO C) can use that safely, without dragging in a definition of the symbol open. Separately, the implementation of open (POSIX) can be defined in its own file, also based on the internal open.)

This approach is fairly general can be adopted with the existing setup as well. In fact, I can image that irrespective of the approach we take, we will end up with patterns like this.

In D67867#1691597, @sivachandra wrote:

In D67867#1688779, @jyknight wrote:

In D67867#1688556, @sivachandra wrote:

The objcopy step is required to avoid putting mangled names with the alias attribute. If there is any other way to achieve the same thing, I am open to it.

Ah, I see! I'd suggest using extern "C" instead. There's no need for these be C++-mangled -- you can simply use a name prefix instead. E.g., if you define it as extern "C" __llvm_libc_strcpy(...) {} then it's trivial to make the strcpy alias without objcopy magic.

Yes, this solution was considered. But, it does not really solve the problem you brought up; we will still need to make the alias weak.

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

In D67867#1688556, @sivachandra wrote:

The objcopy step is required to avoid putting mangled names with the alias attribute. If there is any other way to achieve the same thing, I am open to it.

In D67867#1687128, @theraven wrote:

In D67867#1686112, @jyknight wrote:

In D67867#1686056, @hfinkel wrote:

Maybe everything is fine, but given this setup, does anyone see any potential problems with compiling these functions for nvptx? I'd like to eventually see a mode where we compile an appropriate subset of these functions for GPU targets -- either in bitcode form for linking with our device-side OpenMP runtime library or as a native object -- to provide a more feature-complete offloading environment.

The one thing that caught by eye was the use of the section attribute, and I was curious what nvptx does with that. As far as I can tell, perhaps the answer is nothing.

Then I think this scheme won't work, since the point of the sections is to enable the creation of the global symbols post-build.

E.g., I think the idea is that the main implementation defines the function with C++ name __llvm_libc::strcpy(char *, const char *), and places the code in the .llvm.libc.entrypoint.strcpy section. And then another tool comes along and iterates the llvm.libc.entrypoint sections, and adds global symbol aliases for each one.

That scheme feels probably over-complex, IMO, but I don't have an concrete counter-proposal in mind.

For what it's worth, FreeBSD libc does a similar namespacing trick in C. The internal symbols are underscore prefixed and they're exported as aliases (typically as weak aliases, to allow them to be preempted by other implementations, and to explicitly give names to callers for the preemptible and non-preemptible versions). Making symbols preemptible isn't really possible with PE/COFF, because the linkage model has a stronger concept of where definitions come from than ELF (at least, in the absence of symbol versions in ELF). On ELF platforms, we should support symbol versions as early as possible, because adding them later is an ABI break, even if we change no code.

In D67867#1686056, @hfinkel wrote:

Maybe everything is fine, but given this setup, does anyone see any potential problems with compiling these functions for nvptx? I'd like to eventually see a mode where we compile an appropriate subset of these functions for GPU targets -- either in bitcode form for linking with our device-side OpenMP runtime library or as a native object -- to provide a more feature-complete offloading environment.

The one thing that caught by eye was the use of the section attribute, and I was curious what nvptx does with that. As far as I can tell, perhaps the answer is nothing.

In D67867#1683099, @sivachandra wrote:

I did consider such a layout. However, the same kind of distinction can be achieved by two things:

1. Next to the implementation and/or target listing for a function, call out the standard/extension that prescribes it.
2. When composing the target for libc.a for a platform, group the functions per standards/extensions they come from.
   
   Another point which made me not pick this layout: I agree that as a catalog, the structure you are suggesting could be more meaningful. But as a developer, my mental model is much simpler if all the functions from a header file are grouped in one place, irrespective of the standard they come from.

Some high level comments on your filesystem layout standard:

Ping.

(See https://reviews.llvm.org/D67983 for the proposed behavior change.)

Note that the test-case diffs are on top of https://reviews.llvm.org/D67982, which I split out to make the actual change in behavior in this commit clearer.

Sparc change looks good.
Don't know about the MIPS one -- seems like probably something upstream should be creating a Constant instead of a TargetConstant, rather than converting it there?

The code is attempting to ensure that you don't use RBP as a global register unless RBP is actually a reserved register (being used as the frame pointer in the function.) That's a reasonable goal, but maybe there's another better way to be doing it.

In D65280#1649062, @hans wrote:

Can we not get the entire thing merged? I'd really like that... I think the patch is actually really close. I have a bunch of comments below but they're all pretty boring in reality.

It's really late in the process -- as in I'm only really waiting for this and one other bug -- so I'm hesitant to take a large change, but I also don't really understand exactly what's at stake here.

numbers for cacheline size.