In D147212#4234854, @mstorsjo wrote:

I'm assuming the load/store ops are getting generated by expanding a PUSH/POP, and not some other transform? (So the offset will actually reliably be 4, in practice?)

Not sure actually - if looking at the output of -print-after-all here, the t2STR_PRE appears as such directly, not as a transformation of something else - I don't directly see where it comes from (other than printf instrumenting the code in Target/ARM to see which reference to t2STR_PRE gets hit).

LGTM

LGTM

LGTM

Would we want to use this intrinsic as part of the code generation for C++ new expressions? See https://github.com/llvm/llvm-project/issues/54878 .

Can you stick the testcase into wineh-opcodes.ll?

The numbers are backreferences of the sort generated by mangleSourceName(), I think. If you nest deeply enough, MSVC stops using them; for example:

I don't still understand how to mangle nested unnamed tags in general.

What does f8 actually mean? There are at least 4 proposed "f8" types, and your "f8" represents none of them.

I think the reason "recoverable" ubsan causes trouble is that it introduces branches that subsequent optimizations can abuse. So without ubsan, we just have an udiv instruction. With ubsan, we conveniently have a branch on exactly the condition that would make the udiv undefined, so we can easily prove control flow doesn't continue after the ubsan handler. Subsequent optimizations take advantage of that, so ubsan "breaks" code. (So the code was never actually correct according to the semantic model, but it was broken in a way the compiler is less likely optimize.)

Please don't use size_t to represent anything other than the size of a buffer allocated by the compiler itself; LLVM supports 32-bit hosts. We generally use uint64_t for the size of LLVM IR types.

There's only one tool in existence that produces archives with an ecsymbols section; with your patch to llvm, there will be two. Corrupted sections aren't actually a thing anyone is likely to run into, outside of random data corruption. Certainly not as part of a normal build process.

The problem with a change like that is that it's not clear what the underlying semantic model is. If we add a flag that says "try to generate code that matches Linux kernel developers' mental model of the underlying machine", or "loop unrolling should try to preserve undefined behavior", or "out-of-bounds memory accesses are well-defined", the semantics are unclear. The point of having well-defined IR is that a developer can tell whether a transform is legal or not based on reading LangRef, as opposed to trying to blindly guess whether a transform is going to break some user's code.

I wasn't sure what to do when the map is invalid. We could treat it as an error in Archive constructor, but I decided to just ignore the map instead assuming that the archive may still be useful.

LGTM

There are limits to how much we can do by just changing the code clang generates... but the two particular cases you mention probably could be "fixed" by messing with the IR generated by clang. Sure, that probably makes sense to pursue. (If you're going to pick an arbitrary value, zero is probably a better choice than "freeze poison".)

The relevant text of the current Itanium ABI (which was updated in https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1)

Yes, but kernel developers would prefer to diagnose issues at compile time when possible, rather that at runtime. Function fallthough is diagnosable at compile time, as this patch demonstrates.

Please explicitly note in the commit message that this doesn't affect other archive-writing tools, like llvm-ar.

LGTM

LGTM

This code seems like it isn't being careful to ensure calls to read16le/read32le/etc. don't read past the end of a buffer. Maybe it makes sense to add some sort of bounds-checked read helper? Maybe could leave it for a followup, though, if it's confusing to mix it in with this patch.

Nevermind, ignore me, I somehow got it backwards; the given example gets improved.

Is this something we should support in MASM as well?

Can you add a codegen testcase? Otherwise LGTM.

LGTM

I'm concerned about the potential for false positives:

Rebased on updated llvm-readobj dumping

Adding more reviewers. (Better for someone who's spent more time with this pass recently to look.)

If I'm understanding correctly, the issue here is that you create a context with shouldDiscardValueNames() disabled, parse some code, then enable shouldDiscardValueNames()? That's a little exotic, but I guess we can accommodate if there isn't a measurable penalty for normal frontends.

LGTM

The patch is currently impossible to read; please don't clang-format the whole file. There's a script clang-format-diff.py that can format just the parts of a file you change, if you need it.

Is the issue here actually specific to global variables? I mean, you can't mark a local variable noalias, but noalias/TBAA metadata can apply to local variables.

LGTM

LGTM

LGTM

Looks fine from a codegen perspective, assuming these are the semantics we want for -ffinite-math-only.

LGTM

I guess it's not any worse than the existing -Watomic-alignment. LGTM.

In some other cases of iterative algorithms like this, we change the algorithm after a few passes to a version that converges faster. For example, you could consider adding extra padding after inserted thunks, in anticipation of the need for more thunks.

In D145726#4190341, @garvitgupta08 wrote:

In D145726#4190316, @nickdesaulniers wrote:

Isn't this only an issue with ancient versions of GNU as? Older than 2.16?

I am able to reproduce this issue with 4.9.0 GNU as.

There appear to be 8 different places that call isTypeConstant(Ty, true); I'd like to see testcases for all of them if it's relevant. (I agree with your assessment for global variables, but not sure about the other places.)

Following reproduces for me (clang from main, Ubuntu 16.04).

I think the point of the hasOneUse check is to avoid a possible miscompile; if a FREEZE has more than one use, all users need to see the same value. So not sure dropping the check is correct in general.

I don't really care what happens if someone uses a __builtin_ prefixed name that the underlying libc doesn't support; user should know the semantics of builtins they use. (Some __builtin names refer to functions the compiler can't realistically provide even if we wanted to.)

LGTM

I'm not really an expert on this pass, but sure, seems fine, assuming @nikic doesn't have any further comments.

Something like that, sure.

I'd prefer to focus the documentation more generically on exceptions, not unwind information specifically. -fexceptions makes code generation change in other ways, and "unwind information" doesn't exist on all targets where exception handling is relevant.

Does using an SVE instruction to access a fixed stack slot ever work? I mean, I guess if you get lucky, the fixed offset is zero, but that should happen rarely. Maybe we'd get better code if we expand out the arithmetic earlier. (Doesn't need to block this patch, just something to think about.)

I think this is going to change what inputs Sema will accept for "p". If that's intentional, please add test coverage. Otherwise, please make a narrower change.

LGTM with a couple minor comments

In D145275#4168967, @clin1 wrote:

You're right, this IR won't come out of clang --- it's an artifact of a function shrinkwrapping project that someone was working on. The IR seems generically "legal" but was causing a crash later in codegen. The patch avoids the crash.

How do you end up with IR that looks like "@hoist"? clang shouldn't generate IR like that.

Hoisting like this is completely unsafe; dynamic allocation is dynamic for a reason (because the allocation is variable size, or happens in a loop).

If you're going to enable hoisting past a callbr, please add a testcase to ensure we don't hoist a load past a callbr which modifies the memory in question.

LGTM. These are standard C23 functions, and glibc already has implementations; we should be recognizing them.

Maybe worth cherry-picking to 16 branch? I think someone will need to rebase onto the branch for that, though; there was merge conflict on the microsoft-abi-eh-cleanups.cpp change.

From my analysis of the testcase, we called "getAddExpr(SCEV::FlagNUW)" while analyzing the first loop, and that broke the analysis of the second loop. The actual simplification isn't relevant.

rG674099113533

LGTM

In D144541#4157339, @LuoYuanke wrote:

In D144541#4155710, @efriedma wrote:

That register (other register) may also be clobbered in another inline asm

We don't have to use the same register for each inline asm.

The base pointer register is reserved by compiler, we can't reserve too much register?

perhaps because they did char buf[256] = {0} though

In D144889#4156120, @bcraig wrote:

A freestanding implementation doesn't necessarily mean that everything is header-only. It's fine to require linking against a (freestanding) C runtime library. All header-only is fine too though, if you want to make that work.

Architecturally, I don't feel it is required that Clang be the location of all the freestanding headers for C. I think that's fine for the C library in the final toolchain to own those headers. I'm not super familiar with what interface contracts you have between the clang-provided C headers and the C-library provided C headers though.

Historically, the required functions for a "freestanding" C implementation were very restricted. Freestanding headers didn't export library functions, just constants and types.

Seems reasonable to me.

Why it would mess unwinding?