In D149309#4300794, @MaskRay wrote:

D142117 changed(fixed?) the alignment to 4 for x86-32. Any idea why COMPILER_CHECK(alignof(siginfo_t) == alignof(__sanitizer_siginfo)); did not fail for x86-32?

In libgcc, the same macro is used for both _Unwind_RaiseException_Phase2 and _Unwind_ForcedUnwind_Phase2.
_Unwind_ForcedUnwind usages in glibc always terminate and don't unwind stack. We may not need to check
return address on shadow stack in this case.

In D122789#3447413, @pengfei wrote:

In D122789#3446865, @MaskRay wrote:

To kurly (original Gentoo reporter):

printf '#include <smmintrin.h>\n#include <stdint.h>\nuint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }' > a.c

% clang -m32 -march=i686 -msse4.2 -c a.c  # seems to compile fine
% gcc -m32 -march=i686 -msse4.2 -c a.c
% gcc -m32 -march=i686 -mcrc32 -c a.c
In file included from a.c:1:
a.c: In function ‘computeHardwareCRC32’:
/usr/lib/gcc/x86_64-linux-gnu/11/include/smmintrin.h:839:1: error: inlining failed in call to ‘always_inline’ ‘_mm_crc32_u32’: target specific option mismatch
  839 | _mm_crc32_u32 (unsigned int __C, unsigned int __V)
      | ^~~~~~~~~~~~~
a.c:3:69: note: called from here
    3 | uint32_t computeHardwareCRC32(uint32_t Crc, uint32_t Data) { return _mm_crc32_u32(Crc, Data); }
      |                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~

I have some older GCC and latest GCC (2022-04, multilib), gcc -m32 -march=i686 -msse4.2 -c a.c builds while -mcrc32 doesn't.

I suspect we should revert the -mcrc32 change. The __CRC32__ macro may be fine.

Thanks for the infromation. I got the same result. That's weird. I believe at some point, GCC supported -mcrc32. @hjl.tools, did GCC intentionally remove the support for -mcrc32?

In D111185#3046919, @wolfgangp wrote:

In D111185#3046902, @hjl.tools wrote:

In D111185#3046891, @wolfgangp wrote:

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

0001-sanitizer-Include-cet.h-only-if-it-exists.patch3 KBDownload

Thank you. Are you planning to push this patch to main? This is affecting one of our internal bots that's building the main branch, so either maskray's solution or yours would work if it goes into main.

In D111185#3046891, @wolfgangp wrote:

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

In D111185#3045884, @MaskRay wrote:

Thanks!

Instead of creating a new diff, you may change the local git description, then run arc diff --verbatim --head=head 'HEAD^' to update the Differential's subject.

Some intro to arc (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line)

In D110999#3041390, @MaskRay wrote:

[compiler-rt] Support Intel CET

compiler-rt consists of sanitizers, builtins (similar to libgcc), xray, orc (for JIT), and a bunch of other things.
If this is for sanitizer functions which may be called indirectly, using a [sanitizer] tag may be better.

In D110998#3039753, @morehouse wrote:

I deliberately chose not to mask the fallback tag.

It's never valid to access freed memory anyway, so we can keep a full 8 bit tag to reduce the chance of a false negative.

In D108643#2999724, @aaron.ballman wrote:

In D108643#2991995, @hjl.tools wrote:

The choice that high bits are unspecified rather than extended is an interesting one. Can you speak to that? That's good for +, -, *, &, |, ^, <<, and narrowing conversions, but bad for ==, <, /, >>, and widening conversions.

I've added @hjl.tools to the review for his opinions, as he was the primary driver for the x64 ABI proposal. HJ, can you help me out here?

Please follow up x86-64 psABI proposal with any feedbacks:

https://groups.google.com/g/x86-64-abi/c/XQiSj-zU3w8

We don't have feedback yet, @hjl.tools; we're asking for rationale on the behavior of unused bits in the proposed psABI for x86-64. Can you help us understand why the bits are unspecified rather than extended and whether there are potential performance concerns from this decision (before it gets solidified)? Thanks!

The choice that high bits are unspecified rather than extended is an interesting one. Can you speak to that? That's good for +, -, *, &, |, ^, <<, and narrowing conversions, but bad for ==, <, /, >>, and widening conversions.

I've added @hjl.tools to the review for his opinions, as he was the primary driver for the x64 ABI proposal. HJ, can you help me out here?

Please open a binutils bug with a testcase and CC me. I will take a look.

In D102446#2758313, @MaskRay wrote:

LGTM.

In D99708#2664372, @craig.topper wrote:

In D99708#2664351, @hjl.tools wrote:

In D99708#2664218, @craig.topper wrote:

In D99708#2664164, @hjl.tools wrote:

In D99708#2664076, @LuoYuanke wrote:

In D99708#2663989, @craig.topper wrote:

A user interrupt is different than a regular interrupt right? It doesn't make sense that we would change the behavior of the interrupt calling convention just because the the user interrupt instructions are enabled. That would occur just from passing a -march for a newer CPU wouldn't it?

Maybe need support another attribute "attribute ((user_interrupt))" for functions? However this is what gcc does (https://gcc.godbolt.org/z/8ojTMG6bT).

Since there won't be both user interrupt handler and kernel interrupt handler in the source, there is no need for another
attribute. We discussed that kernel might need to use UINTR instructions. We decided that kernel could use inline asm
statements if needed.

So if write kernel code and compile with -march=haswell today, I get IRET. If tomorrow I change my command line to -march=sapphirerapids, now my kernel interrupt code generates user interrupt instructions. That seems surprising.

-mcmodel=kernel should disable uiret.:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99870

That makes sense. Can we put that in this patch?

In D99708#2664218, @craig.topper wrote:

In D99708#2664164, @hjl.tools wrote:

In D99708#2664076, @LuoYuanke wrote:

In D99708#2663989, @craig.topper wrote:

A user interrupt is different than a regular interrupt right? It doesn't make sense that we would change the behavior of the interrupt calling convention just because the the user interrupt instructions are enabled. That would occur just from passing a -march for a newer CPU wouldn't it?

Maybe need support another attribute "attribute ((user_interrupt))" for functions? However this is what gcc does (https://gcc.godbolt.org/z/8ojTMG6bT).

Since there won't be both user interrupt handler and kernel interrupt handler in the source, there is no need for another
attribute. We discussed that kernel might need to use UINTR instructions. We decided that kernel could use inline asm
statements if needed.

So if write kernel code and compile with -march=haswell today, I get IRET. If tomorrow I change my command line to -march=sapphirerapids, now my kernel interrupt code generates user interrupt instructions. That seems surprising.

In D99708#2664076, @LuoYuanke wrote:

In D99708#2663989, @craig.topper wrote:

A user interrupt is different than a regular interrupt right? It doesn't make sense that we would change the behavior of the interrupt calling convention just because the the user interrupt instructions are enabled. That would occur just from passing a -march for a newer CPU wouldn't it?

Maybe need support another attribute "attribute ((user_interrupt))" for functions? However this is what gcc does (https://gcc.godbolt.org/z/8ojTMG6bT).

In D78564#2459571, @LiuChen3 wrote:

In D78564#2457869, @hjl.tools wrote:

Does this fix https://github.com/golang/go/issues/42440?

No. I think this patch can only fix part of the issue.

Does this fix https://github.com/golang/go/issues/42440?

In D16474#2421552, @hvdijk wrote:

In D16474#335360, @hjl.tools wrote:

LLVM ERROR: Cannot select: 0x29507a0: ch,glue = X86ISD::TLSADDR 0x28fc140, TargetGlobalTLSAddress:i32<i32* @x> 0 [TF=7]

This still happens with current LLVM. If we can push the fix for that first (presumably you fixed this already on your old branch), we could then include your test case in this diff.

My old x32 patches are at https://github.com/hjl-tools/llvm-x32-old/tree/hjl/x32/2016-09-01

In D91338#2395907, @pengfei wrote:

It is required for ILP32 since many system calls are shared between ILP32 and LP64.
Here is my x32 port from 2016.

Thanks H.J. It's amazing you already did many works on x32 support. But these patches seem not been merged to mainline. What's the reason for not merging them? Does that mean the trunk code still has some flaws on x32 ABI supporting?

In D91338#2395608, @hvdijk wrote:

In D91338#2395497, @pengfei wrote:

Hi Harald, thanks for thoroughly answering my questions. I still have doubts intention of this patch. Is there any bug related to it?

I run an x32 system. Things work well for things built with GCC, but break badly when using LLVM, because the ABI as implemented is incompatible between GCC and LLVM, as GCC-generated code will sometimes assume that the high bits of pointer parameters have been zeroed out as required by the ABI. This is especially visible when using Rust applications, as the Rust compiler is LLVM-based but most of the rest of my system is built with GCC. Pretty much any non-trivial Rust application crashes, for instance ripgrep, or the Rust compiler itself.

IMHO, the ILP32 mode is designed for performance which always assumes the address bit 63~32 all zero and uses 32 bits register to reduce code size.

In D79617#2045552, @rsmith wrote:

I would like a specification for this header to be added somewhere. We shouldn't be implementing random things with no specification. (Suppose someone claims that our <cet.h> is wrong in some way. How would we know whether they're right?)

Ideally, I'd also like this header to be installed somewhere where we look for assembler-with-cpp preprocessing but not for regular compilation; it doesn't make sense to me to pollute the header namespace for all C and C++ compilations with a header that is not meaningful in C and C++. But knowing whether that change is correct depends on having, you know, a specification.

In D79617#2028862, @xiangzhangllvm wrote:

Refine the Macro _CET_ENDBR

In D78564#1995840, @LiuChen3 wrote:

In D78564#1994943, @craig.topper wrote:

I'm not sure this is right. _m512 is just a typedef to a 64 byte vector_size attribute. This patch changes the behavior of using 64 byte vector_size on an sse/avx target. Prior to avx512 existing an avx target would pass a 512 bit vector 32 byte aligned. It does't make sense to me to change the alignment on an avx target just because avx512 exists, but isn't enabled.

If we use an library function which passing __m512 on stack(such as variadic function) compiled with avx512 but the caller compiled without avx512, this will cause run fail. But actually, when parameters passed by registers, it will cause run fail too because caller and callee use different register. Therefore, it is hard to say whether it is reasonable to align to 64 bytes.

Look good to me.

In D76900#1957289, @hjl.tools wrote:

It looks very confusing. Can you create a diff against master branch, not your last change?

In D76900#1956612, @xiangzhangllvm wrote:

1. Sync with HJ's large code changes in https://github.com/hjl-tools/llvm-project/commit/6a85ba472143c91e5686e1f0faf7fea7b4f0e350
2. Add Eli's test in https://bugs.llvm.org/show_bug.cgi?id=45364 to llc test and JIT test.

For Large code model:
The compiler is required to use the movabs instruction, as in the medium code model, even for dealing with addresses inside the text section.
Additionally, indirect branches are needed when branching to addresses whose offset from the current instruction pointer is unknown.

In D76900#1953937, @xiangzhangllvm wrote:

I find your patch will not deal with internal function.
I tested GCC, It really will generate endbr for static functions:

gcc version 9.3.1 20200317 (Red Hat 9.3.1-1) (GCC)
 [xiangzh1@gnu-tgl-1 ~/tmp]$cat t.c
extern int foo2(int a);

extern int foo2(int a)
{
  return a*6;
}

static int foo3(int a)
{
  return a*7;
}

int foo(int a){
//  int (*pf3)(int) = foo;
//  int b = foo2(a) +foo3(a) + pf3(a);
  int b = foo2(a) +foo3(a);
  return b;
}
 [xiangzh1@gnu-tgl-1 ~/tmp]$
[xiangzh1@gnu-tgl-1 ~/tmp]$gcc -fcf-protection t.c -S -o t.s

In D76900#1951695, @xiangzhangllvm wrote:

@efriedma Put the https://bugs.llvm.org/show_bug.cgi?id=45364 test into X86/indirect-branch-tracking.ll @test5

In D76900#1949402, @xiangzhangllvm wrote:

If "1 Check CET in JIT instead of checking -fcf-protection-branch" can't let VNC passed. The crash must happen at "Take address of a internal function."

This is not a IBT problem only for JIT, but also the static llvm compiler, e.g. llc.
And it may unable to track these calling through function address, because these function address may be calculated out in runtime or written in a big table.

In D76900#1949020, @xiangzhangllvm wrote:

In D76900#1948990, @hjl.tools wrote:

2, Do you mean here we need add JIT test for this patch ? Check endbr in JIT generated code?

My changes fixed the VNC crash. But there is no testcase to show why my patches are needed.

Your tests failed on CET machine
and the existing llvm unit tests also failed the on the CET machine
They are the same reason for lack of endbr* in JIT.
So I think the existing llvm unit tests is enough for this patch.

In D76900#1948978, @xiangzhangllvm wrote:

In D76900#1948503, @hjl.tools wrote:

Please take a look at

https://gitlab.freedesktop.org/mesa/mesa

to see how LLVM JIT is used and extract a testcase from it.

1, Does it in your commits? I find 2 your commits without the JIT tests.

Please take a look at

In D74006#1860062, @MaskRay wrote:

.section .foo,"o",@progbits,foo  # first in the output section .foo due to stable sort
.byte 0

.section .foo,"o",@progbits,bar  # second
.byte 1

This is what lld currently does. It will be nice to get a sign-up from GNU ld. An alternative choice is that users should not rely on the order. I'm inclined to define an order here.

In D58102#1421810, @emaste wrote:

@xiangzhangllvm, a question for you not directly related to the patch but I suspect you might be best suited to find an answer: how do we test CET support today? We started trying to add CET to FreeBSD some time ago but have no way to test any work that we do.

In D58102#1414695, @xiangzhangllvm wrote:

Add -z force-cet option to force enable CET.
But if we do not use this option, CET will also try to work unless meet the conditions of no satisfaction.

In D58102#1412090, @peter.smith wrote:

Thanks for putting this feature forward. I've not had a chance to go through everything in detail but I thought it would be important to mention that AArch64 has a similar set of features (Pointer Authentication PAC and Branch Target Identification BTI) that are going to use .note.gnu.property sections with GNU_PROPERTY_AARCH64_FEATURE_1_AND (same meaning as GNU_PROPERTY_X86_FEATURE_1_AND), with two associated feature bits GNU_PROPERTY_AARCH64_FEATURE_1_BTI and GNU_PROPERTY_AARCH64_FEATURE_1_PAC. AArch64 does need a modified PLT entry to make this work but it doesn't use a .splt, in effect an extra instruction at the top of the PLT if BTI is used and one at the end if PAC is used, or both if both BTI and PAC are needed. Given that we will have at least two targets implementing a similar mechanism but with target specific details then we'll either need to make it my responsibility to generalise the .note.gnu.property implementation so that it can support both AArch64 and X86, or we make it generic from the start. The main difference for AArch64 is that there are two independent feature bits to track.

In the case of AArch64 it is important for the program loader to only enable the BTI/PAC feature for the process if the whole program has been compiled/assembled to support it. Our prior experience with assembler files in particular is that it is very easy to get a single .s file added to a build that is harmless but doesn't have the .note.gnu.property set properly and a single one of these would be enough to clear all the features. Our thoughts were to add a command line option to force generation of the appropriate PLTs and the .note.gnu.property in the output file but to warn if an input file doesn't have the .note.gnu.property flag.

In D58413#1406201, @rnk wrote:

Of course the test fails, this always returns 0. Recommitting with the test disabled on Windows seems reasonable.

It looks like that ReadBinaryNameCached doesn't work on Windows.
Someone who is familiar with Windows should take a look.

See:

In D53919#1287510, @echristo wrote:

In D53919#1282994, @hjl.tools wrote:

In D53919#1282952, @efriedma wrote:

With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it would be helpful):

Please try clang 2.6 on both testcases.

From the releases:

23 Oct 2009 2.6

... why would you care about a 9 year old version of clang?

In D53919#1282952, @efriedma wrote:

With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it would be helpful):

In D53919#1282811, @efriedma wrote:

No, AVX512 wasn't even announced when clang 3.3 came out.

In D53919#1282797, @efriedma wrote:

I just tried clang 3.3, and as far as I can tell it behaves the same way as trunk.

If the argument is that we should be compatible with gcc on Linux, that's fine, but we should explicitly state that in a comment.