Added some comments to explain the rationale behind CheckPointer and SeePointer. If it looks okay, could you accept the patch?

update the comment.

add comments to explain CheckPointer and SeePointer.

@dyung I just pushed the fix to the 'main' branch (https://github.com/llvm/llvm-project/commit/183d075055c591dedead7ece972f1bdea611aa3b). Please check it out. Thanks for reporting!

Sorry, I double checked. '-fstack-protector -fno-stack-protector' will not result in warnings. So the patch LGTM. So gentoo people can add -fno-stack-protector to suppress warnings.

@ast With this patch, gentoo clang compilation will hit the warning even if people appends -fno-stack-protector. Is this okay? In general, if the option is '-fstack-protector -fno-stack-protector', we should not issue warning, right?

@compnerd could you also take a look at this patch?

LGTM. Thanks!

LGTM. I will add // REQUIRES: bpf-registered-target in the test to ensure it only executed with bpf target.

Please separate the patch to clang and llvm part.

Somehow, CCACHE does not work for me, which has an error message like missing 'malloc/malloc.h'. I used the following scripts without CCACHE.

$ cat build_llvm_debug.sh
cd llvm-project/llvm/build

Otherwise, the patch LGTM.

Thanks for the code simplification. LGTM.

• improve the bug description in release note.

@aaron.ballman or @dblaikie I have addressed the comments, could you take a look again?

• Add the bug fix description in Release Note
• Actually compare debuginfo btf_type_tag output instead of marking 'expected-no-diagnostics'.

In D136807#3888317, @aaron.ballman wrote:

The changes generally look good to me, but can you add a release note for the fix as well?

Yes, will add the bug fix brief description to clang/docs/ReleaseNotes.rst

In D136041#3863748, @dblaikie wrote:

Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning?

@aprantl The following example shows the difference between gcc and clang w.r.t. reserved names.

Thanks. The change looks good to me from BPF perspective as it fixed the regression in https://github.com/llvm/llvm-project/issues/57872.

• simplify test case, add more CHECK's and add comments to explain the test.

In D134705#3818657, @dblaikie wrote:

SGTM - could you also add some CHECKs in the test that show the enumeration was added to the enumerators list on the CU (& you can probably test with just one enumerator - don't need two in each test for instance)

In D77804#3804930, @RKSimon wrote:

@yonghong-song Please can you raise this as an issue and include the IR as well? AFAICT this is a perf regression, and not an actual bug

Hi, we found a regression with some bpf code with this patch. The following shows the problem:

I hacked the kernel x64 jit part, with the following jit for sext load, it seems working okay from jit perspective.

+/* LDX: dst_reg = *(s8*)(src_reg + off) */
+static void emit_ldx_sext(u8 **pprog, u32 size, u32 dst_reg, u32 src_reg, int off)
+{
+       u8 *prog = *pprog;
+
+       switch (size) {
+       case BPF_B:
+               /* Emit 'movsx rax, byte ptr [rax + off]' */
+               EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xBE);
+               break;
+       case BPF_H:
+               /* Emit 'movsx rax, word ptr [rax + off]' */
+               EMIT3(add_2mod(0x48, src_reg, dst_reg), 0x0F, 0xBF);
+               break;
+       case BPF_W:
+               /* Emit 'movsx rax, dword ptr [rax+0x14]' */
+               EMIT2(add_2mod(0x48, src_reg, dst_reg), 0x63);
+               break;
+       }
+       emit_insn_suffix(&prog, src_reg, dst_reg, off);
+       *pprog = prog;
+}

All major architectures support sign extension load (x64, arm64, sparc, webassembly, lanai, etc.)

add two llvm/test/CodeGen/BPF tests.

but if the function is always_inline it can have more than 5 arguments, right?

Kernel changes were assuming at most 2

The compiler only counted the total number of registers won't exceed 5. You could have 5 struct arguments (each one register), or 2 struct arguments (with two registers each) and one integer register.
Whether to pass by value or by reference is a ABI issue, not related to the number of arguments. Checking whether arguments are enough or not is left to BPF backend lowering.
This is an example,

[$~/tmp3] cat t3.c
int foo(int a1, int a2, int a3, int a4, int a5, int a6) {
  return a1 + a2 + a3 + a4 + a5 + a6;
}
[$ ~/tmp3] clang -target bpf -O2 -S -emit-llvm t3.c
[$~/tmp3] clang -target bpf -O2 -S t3.c
t3.c:1:5: error: defined with too many args
int foo(int a1, int a2, int a3, int a4, int a5, int a6) {
    ^
1 error generated.
[$ ~/tmp3]

In D132144#3732685, @anakryiko wrote:

Are there any restrictions about number of struct arguments that can be passed in? Kernel changes were assuming at most 2, should we have a test that tests passing 3 structs that fit into 5 input registers and another test that passes 3 structs that do not fit in 5 input registers? And what should be the behavior in the latter case -- compilation error or switch to "pass by reference"?

In D131468#3708977, @pengfei wrote:

D124435 is going to change the assumption :)

In D126839#3554474, @d-e-s-o wrote:

In D126839#3554125, @yonghong-song wrote:

Also let us merge it unti kernel side change is in reasonable shape.

From what I understand it may take several days or some other not-precisely specified time until "nightly" builds, which we use in the BPF CI, pick up the change. Hence, I was under the impression that we should attempt to merge it sooner rather than later once it's ready to be merged. The kernel changes are in good enough shape to show feasibility, so I don't really see why not, but it's your call.

LGTM. But add the reference (https://reviews.llvm.org/D126838) to the commit message to indicate which 'earlier change'. Also let us merge it unti kernel side change is in reasonable shape.

@compnerd provided a better fix https://reviews.llvm.org/D126093, so I am going to close this one.

Sorry. I missed this patch somehow and did another patch https://reviews.llvm.org/D126223. My patch skipped TL.setNameLoc(DS.getTypeSpecTypeNameLoc()); which probably not correct. Accept this patch and abandon my patch D126223.

@compnerd Thanks for reporting. I will take a look as soon as possible.

• also set signed enum for dwarf::DW_ATE_signed_char int.