I find this warning confusing. I find a4 to be perfectly expected. IMO this warning should be applied only, if the effective value of the expression is not the same as in the modulo-n arithmetic. This means that if (-x) is explicitly or implicitly cast to a less wide unsigned type, it should not warn. It would consider a warning for the case of using (-x) if integer promotion rules makes it negative though. The question is, how to best patch around the warning though. What options does MSVC have for that? I.e. what equivalent expressions do not trigger this warning?

Correct. The protected name is double underscore as both suffix and prefix.

Please check the history of the file for some of the problems with the redefinition. I'm quite against this change.

Every system call has a public and internal variant. The former might be replaced by libpthread etc for thread cancellation support, but that's a different topic.

Yes, it is optional, but on most architectures, the builtin variant is much cheaper. That said, I'm not sure what the situation is on SPARC with the necessary register window flush.

I don't understand why most of this symbols don't reference the plain system call directly, i.e. _sys_read etc.

There is one user of builtin_setjmp/builtin_longjmp that should be kept in mind: Ruby.

Is there a reason for defining them? As in: does anything outside libunwind use them? I haven't seen such software yet.

Why do we need to allocate memory in this case at all? I.e. why can't this just be:

if (S.empty())
  return StringRef("", 0);
...

If a build against compiler-rt works, that's ok. It wasn't clear from the diff.

I don't understand the desire for this logic. Why can't wasm override the rest of the types if it wants to have something special?

This hard-coding seems to be counter-productive, it could be compiler-rt just as well.

There are two different considerations here:
(1) Create less target code
(2) Create less IR

Depends a bit on the platform, __cxa_atexit on most modern ELF systems, fallback to atexit. If the global dtor is run too late, it smells like a missing library dependency. They are executed in topological order after all.

Can this ever end up in a shared library? If yes, please use the normal logic for creating a global destructor. atexit is not very friendly to dlopen...

Both needs a test case :)

It seems to miss most of the interesting checks, i.e. crt files. Compare with any of the entries on netbsd.c for example.

This is absolutely not how the clang driver is supposed to work. No conditional compilation.

Looking further, at least on NetBSD libgcc seems to always include both the "normal" and the .rem/.urem routines. While we currently don't replace __umodsi3 and friends, that looks more like an oversight on our part.
I asked the Sparc folks and they can't remember any special reason for why .urem should be used. I.e. it follows the normal Sparc ABI.
As such, I'm mostly ambivalent on this change and the rest of the block.

Not what I mean. Certain platforms like Sparc and SH link a copy of certain routines into every DSO. This is the so-called milli code. They sometimes use special calling conventions as well. That's different from the "normal" helper routines in libgcc, which are shared by all libraries.

That would be the milli code version, wouldn't it?

Please update the summary / title

After a careful review of newer GCC / libgcc and the assembler annotations from LLVM, I have come to the following conclusions:

This still needs a test case?

This looks sensible, but I don't know what PoisonShadow will do for the rest of the memory block.

"-z textrel" can also be used in build instructions if some platforms will need it, even if not all of them do.

Yes, if that option is enabled explicitly or implicitly in the frontend, the expectation is that all compiler-generated functions have uwtable as well.

Given that .eh_frame sections can be used to create backtraces i.e. from signal handlers, this seems to be undesirable in the generality. Shouldn't this attribute be conditional on whether functions are normally supposed to have unwind data?

I'm back to the point where I can't reproduce the problem :( Can we start providing an actual failing test case? It's annoying to debug a problem when you can't reproduce it.

Things are different for a libgcc-based toolchain and a compiler-rt based toolchain.

Can you make sure that we handle the older ARM versions correctly as well, i.e. v4, v5 and v6? I take it we still have test cases for the arm <-> thumb transition? That's the one part of the triple logic that is really non-trivial.

The "struct object" is an implementation detail of the unwind implementation. You are guaranteed historically to get at least 8 longs / 8 pointers for internal use statically allocated in each object. What is stored inside is up to the unwind implementation.

GCC supports -mbss-plt to get the legacy behavior. Not sure if anyone actually uses it though.

Given that some people like to post-process assembler files, using the section symbol directly is a bad idea. Adding the local symbols is fine.

Oh, we certainly should never be hitting an assertion on front-end flags. As such, there is a problem to fix here. I still maintain that the combination of flags is non-sense, so the question is:

It should be kept in mind that secure PLT is desirable for certain cases with non-position independent code as well. Even in static binaries it can be desirable... But that is for a follow-up patch.

IMO we should explicitly error out. That combination is nonsense to me. Creating useless JSON database fragments is not an improvement.

The difference is that modsi3 etc are all paired instructions. A backend should not be lowering to one of them if a real division instruction exists and it should be consistent in the lowering.

ARM and x86 implement different chars, don't they?

We tried to keep the condition simple. I.e. does the compiler on any of those platforms ever use the libcall? If not, it is IMO not worth the complexity.

Please stop adding complexity to doctor around the symptoms. There are two real fixes here and this change doesn't help with either:
(1) Emit cross-section pointers as indirect. This increases the binary size, but otherwise ensures that any linker can create read-only .eh_frame on MIPS.
(2) Teach lld on MIPS to properly reassemble the DWARF instructions, similar to what GNU ld can do. The latter is a bit stupid and needs a good kick to work properly, but this is the correct approach forward.

This is not acceptable. If anything, the encoding should be switched to indirect, but that should already be the case.

I really don't like ignoring options that are supposed to provide actual functionality. Most of the other options are for pointless fine tuning and workarounds for broken gcc behavior in ancient versions.

I really, really dislike this patch. It is using very blunt force to workaround a GCC bug. The comment is too verbose as well. Please try the following change from NetBSD instead:

Good enough for me.

Do you see the comment just following the code? The patch completely violates that basic design principle. It would be perfectly sensible to hard-code a list of dumb terminals and explicitly default to no colors for them. The reverse (hard-coding a list and assuming it is fine for everything else) is not.

That's no excuse for making the situation even worse.

I completely disagree with this approach. A lot of GNU tools (including GCC) are completely broken. We shouldn't follow them. There are a lot more terminals around than just "dumb", "xterm" and "linux". It is completely non-acceptable to just assume ANSI escape sequences work. If Android doesn't ship a usable terminfo implementation, I consider that an Android bug. Wouldn't be the first portability nightmare with Android.

This works in 32bit mode as well? I'm suprised.

I'm not really a fan of linking libutil into all binaries. Why is this code using forkpty in first place and not posix_openpt/grantpt?

Instead of computing and storing the modulus directly, it is likely better to precompute the inverse and use that to improve the performance of the operation in first place. Consider using fast_remainder32 and associated functions.

So the next steps if you have the time would IMO be:

Split into verbose conditional register names into a separate function. We likely want to remove them going forward as they are a specific feature of the Darwin assembler and not wildly supported.

The public interface for obtaining the TLS storage is the combination of reading the DTV vector of a thread in combination with dl_iterate_phdr to find the size of the TLS block of a specific module. That gives you all that you need to know. It is important to keep in mind that the vector can be initialized lazily, so __tls_get_addr and friends will have to be intercepted to update the global view.

I really dislike this direction. fallocate can double the amount of disk IO and increase cache trashing, especially when linking large programs with debug information. Keeping more things in memory doesn't sound like an actual improvement either. If the goal is really only to improve the diagnostics in tools, I think a better idea would be to figure out a good way to handle this from a SIGBUS handler based on the passed in siginfo_t.

No need for a custom container, just allocate the vector dynamically and free it when it becomes empty.

No, __cxa_atexit will always reference the DSO handle. That exists even in the main executable.

Is there any reason why keeping at_exit and __cxa_atexit handling merged? They are pretty much disjunct code paths, especially since the at_exit stack means that the real at_exit can be used.

In D39079#905468, @rnk wrote:

In D39079#905454, @joerg wrote:

It also increases the pressure on the branch predictor, so it is not really black and white.

I don't understand this objection. I'm assuming that the PLT stub is an indirect jump through the PLTGOT,
not a hotpatched stub that jumps directly to the definition chosen by the loader. This is the ELF model
that I'm familiar with, especially since calls to code more than 2GB away generally need to be indirect anyway.

In D39079#905396, @rnk wrote:

In D39079#905372, @joerg wrote:

Why again is this a good idea?

It saves the direct jump to the PLT, reducing icache pressure, which is a major cost in some workloads.

Let me phrase it differently. What is this patch (and the matching backend PR) supposed to achieve? There are effectively two ways to get rid of PLT entries:
(1) Bind references locally. This is effectively what -Bsymbolic does and what is breaking the ELF interposition rules.
(2) Do an indirect call via the GOT. Requires knowing what an external symbol is, making it non-attractive for anything but LTO, since it will create performance issues for all non-local accesses (i.e. anything private).

Why again is this a good idea? This is an even worse hack than -Bsymbolic, the latter at least is visible in ELF header without code inspection. This is breaking core premises of ELF.

Almost.

In D39016#903937, @alexandreyy wrote:

Because it is not possible to change the cl option by LLDB.

This is even worse. You can't new[] and then free(). Please follow the suggestion on just embedding the prefix directly, if desirable.

Even a full static binary will have a PLT when IFUNC is used. As such, a linker has to deal with conversion between direct and PLT branches anyway.

A PLT is used not only by PIC code. It is required for all dynamic entry points and that's not limited to PIC. It's not even limited to dynamically linked binaries. There is no support for the embedded ABIs as I said before. I'm going to stop responding since it is rather pointless now. My objection stands.