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.

In D57450#1641190, @lenary wrote:

@jyknight I hear where you're coming from. I'll see what I can do about the psABI document.

In that ticket, it's mentioned that the Darwin ABI explicitly says that non-power-of-two atomic types should be padded and realigned, but I cannot find any documentation explaining this. That would be useful, given presumably GCC does have to pad/align on Darwin.

That GCC and Clang differ in handling of Atomics is a really unfortunate, longstanding issue.

Seems reasonable.

Doing a websearch for "xtensa isa", I do see that documentation does seem to exist: https://github.com/eerimoq/hardware-reference/blob/master/esp32/xtensa%20Instruction%20Set%20Architecture%20(ISA)%20Reference%20Manual.pdf.

+1 for doing this. I started looking at fixing this when I modified the printer to print proper labels for numbered basic-blocks (instead of comments), but I didn't do so because of the amount of test churn was off-putting.

With this new pass, I think we should be able to remove the code handling the objectsize and is.constant instrinsics in CodegenPrepare, FastISel, SelectionDAGBuilder.cpp, and IRTranslator. Which sure is a nice cleanup.

I think the pass needs to handle the removal of any remaining llvm.objectsize, as well, so that llvm.is.constant(llvm.objectsize(...)) continues to return true -- even if the object size cannot be determined.

.

I don't really want to promise any particular version, because nobody is really checking that particular versions of git work -- the way our scripts continue work with old versions is when developers use them and report a problem. The only real requirement here is "whatever the oldest version someone's actually used recently".

Given the reference to the bug, seems fine to me to commit an xfail here since nobody is actively working to fix this right now.

I'm not sure the history behind why these were added as default-on warnings....they don't really seem appropriate as default warnings to me, either.

I fear it is necessary: at least it matches documented behaviour of both the Sun/Oracle Studio compilers and gcc.

Is this really necessary? Users don't typically pass -std= to the driver for linking anyways (what do you even pass if you've compiled both C and C++ code?) so this seems a rather odd way to control behavior.

In D65123#1596983, @labath wrote:

In D65123#1596935, @teemperor wrote:

Thanks for restoring this, but I'm a bit confused how this has happened. My original diff didn't remove these folders, but it seems when I committed the diff via arcanist and git llvm it also removed these other folders. I assume git llvm is when translating the change to SVN not just removing a folder but also all folders in the same directory?

Interesting.. I wouldn't be totally surprised by that as git-llvm does some magic to svn check out only the files/directories that have been modified. Combining that with the different treatment of directories in svn and git (explicit modelling vs. implicit "it exists if it contains a file"), I can see how things might go south in some corner cases like this. But that's just speculation -- I'm generally trying to avoid git-llvm as it is too magical for my taste. Maybe @jyknight or @mehdi_amini can shed more light on this?

The code you reference in GCC is only for trampolines, while this function is used to implement builtin_clear_cache. AFAICT GCC is just wrong to implement this as a no-op.

LGTM.

I agree, it should have one, especially once we stop supporting separate checkouts, and hopefully can remove redundant copies in other places.

Actually linker scripts do support metacharacter escaping (although, it seems lld does not support this correctly, yet).

Comment updates SGTM.

I like it. Some minor suggestions for a few extra comments, but other than that LGTM.

I don't think this was correct (where by "correct", there, I mean "what GCC does", as this patch is intended to match GCC behavior).

I don't really have much to say about this, and the patch is probably fine, but I do note that most of the other accessors on this class also return mutable objects.

One of the other suggestions was to pass a _type_ as a parameter to byval. IMO that would be the nicest idea (but I don't know if it's infeasibly difficult?)

Sorry for dropping this, a couple more comments, then I think it's good.

In D62268#1512672, @ldionne wrote:

This LGTM.

When I did the refactor, all the code was only using -isysroot (and never accessing --sysroot), so I thought only -isysroot was relevant on Darwin. Seems like I was wrong.

There shouldn't be an empty string ("") in the asm output -- that should be a reference to the "l_yes" label, not the empty string. That seems very weird...

Ping.

Ping.

Thanks!

If you change to condition on GLIBC instead of linux, I think this is fine to commit (although it still seems unfortunate to me that we're carrying an abandoned codebase which depends on weird broken stuff like this...)

Looks reasonable to me.

Can you please write a test case for this? There's some existing sparcv9 spill tests in llvm/test/CodeGen/SPARC/64spill.ll.

This commit message is rather bare-bones -- I hope that the ultimate commit message will be updated to contain more than just a reference to the RFC thread.

This patch is not correct -- the crash is not with varargs functions specifically, llvm also crashes for a function declared to explicitly take a float/double. The Win64 calling convention code assigns values to xmm registers, but it should not when sse is disabled.

After looking at this a bit more, I realized that this is still insufficient. There's *many* more target-specific constraints than just x86 "i" which sign extend constant integer arguments -- and we'll need to apply this change to every one of them, in all targets.