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.

• change kflag meaning: kflag = 0 for unsigned and kflag = 1 for signed.
• change the ordering of enum64 val_lo32 and val_hi32.

@Jim Thanks for the updated commit message. Please go ahead to merge to llvm-project main branch if you have write permission. Otherwise, please provide me with peoper git author signature "<name> <email address>" and I can help push the patch for you.

In D125185#3505000, @ast wrote:

a test got lost?

Could you provide proper "<Name> <Email Address>" so I can change author to you when I push the commit to the main branch?

The current 'summary' or commit message is too simple. Could you clarify some motivation why you need to add this instruction?
The change itself looks good to me. The kernel selftest test_verifier does test BPF_MOD instruction and I checked verifier code, BPF_MOD is handled similar to BPF_DIV which should be okay in most cases, so I didn't see a major issue here.

The change looks good to me. Do you have write permission to merge into llvm-project main repo? If not, I can do the merge for you.

please having proper reviewers. Me, @anakryiko @ast are not the right persons to review this patch.

The BPF doesn't really recommend to compile from the assembly code to the object code, so we should be fine here. But there is a test failure test/CodeGen/BPF/inline_asm.ll. Could you take a look?

• create and use isLoadInst() helper function.

• use report_fatal_error instead of assert.

In D123877#3459461, @ast wrote:

assert is a heavy hammer.
can we exit with report_fatal_error instead?

The UseIntegratedAssembler = false is added with the following commit,

commit d2e5157c1f0b1953c5166c1d656ac71e840615a4
Author: Fangrui Song <i@maskray.me>
Date:   Sat Apr 11 10:01:36 2020 -0700

• simplify code with stripPointerCasts

In D123800#3452076, @ast wrote:

The fix to prevent the crash looks good, but what should be the expected codegen for a global variable?
CO-RE needs a constant.
Are we assuming that the global data will be inited with some constant and a later pass will propagate it further into __builtin_preserve,
so at the end it will become a constant?

Ya, typo. memcpy is okay, it is memcmp. I probably just typing memcpy way more times than memcmp :-) Will change.

Indeed, alignment is not used for union type. Your change looks good to me.

LGTM. Maybe @efriedma can look at this as well.

• fix typo
• add a new test case with typedef of volatile type.

In D121715#3397407, @efriedma wrote:

In D121715#3397215, @xbolva00 wrote:

If there is j = j + 1 .. do we warn ?

In D121715#3397237, @xbolva00 wrote:

j++;

Should not we warn in this case? (Currently no warning)

We probably should warn in both those cases; the variable clearly isn't "used" in any meaningful way.

• only handle volatile variables with component assignment.

• change TransformBTFTagAttributedType() implementation with a simple llvm_unreachable() message. I tested linux kernel and its selftests/bpf compilation with clang and found the only TreeTransformation function is called with TransformAutoType() due to __auto_type usage.

@eli.friedman Please feel free adding other reviewers if needed. Thanks!

• add one more _Generic test.

• fix one 'auto' declaration issue
• rework on the test with overloadable attribute.

@aaron.ballman @erichkeane Hopefully I addressed your comments. Could you take a look again?

• use BTFTypeTagAttr instead of Attr in PropertiesBase.td

• further clang-format fixes.

• clang-format fix
• change variable names to be llvm compliant.
• remove Attr from BTFTagAttributedTypeLoc as it can be retrieved from the BTFTagAttributedType
• Also transform the attribute in TreeTransform
• add additional test with _Generic and overloadable attribute

@aaron.ballman Thanks for the review! I will address all your comments and will also add tests with _Generic and __attribute__((overloadable)).

• add a pch test to test serialization with btf_type_tag.
• simplify the code

@erichkeane Just posted a patch to address your comments. Could you take a look? I will try to add more tests, esp. serialization and libclang. If you have any suggestions in these two areas (as I am not familiar with them), it would be great!

• address @erichkeane comments
• further TODO: add more tests

@erichkeane Thanks for the review. I will try to address your comments as soon as possible.

@aaron.ballman @erichkeane Just updated the patch with a new implementation, which added a new BTFTagAttributedType instead of reusing existing AttributedType. It is quite complicated and various parts of clang are touched. I mostly just followed what AttributedType did but considering BTFTagAttributedType is C only so my current implementation may unnecessarily complicate things. I didn't have much test coverage either. It would be great you can give a look and let me know (1). what I am missing, (2). where I can simplify things, (3) a different approach if you see fit, (4). what additional tests I should have. Thanks!

• Create a separate type BTFTagAttributed instead of using Attributed type based on previous discussion

@erichkeane Thanks for suggestion. I will add as an independent type then.

@aaron.ballman Thanks for suggestion. Agree that Having a new Type is a better idea. I guess, I will add BTFTagAttributeType which extends AttributeType to see whether it works or not.

@aaron.ballman ping, did you get time to look at this patch?

fix clang-format issues.