samitolvanen added inline comments.
================ Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 + KCFI_NT_CALL, + KCFI_TC_RETURN, + ---------------- joaomoreira wrote: > I did not revise the entire patch yet. With this said, IMHO, this looks like > an overcomplication of a simple problem. Is there a reason why you really > need specific KCFI_ nodes instead of only embedding the hash information into > an attribute at the Machine Instruction? Then, if hash == 0, it just means it > is a call that doesn't need instrumentation. > > This latter approach will require less code and should be easier to maintain > compatible with other CFI approaches. If the reason is because you don't want > to have a useless attribute for non-call instructions, then you could > possibly have a map where you bind the call instruction with a respective > hash. > > Unless there is a strong reason for these, I would much better prefer the > slim approach suggested. Either way, if there is a reason for this, I would > also suggest that you at least don't name these as "KCFI_something", as in > the future others might want to reuse the same structure for other CFI > approaches. > Is there a reason why you really need specific KCFI_ nodes instead of only > embedding the hash information into an attribute at the Machine Instruction? This implementation is similar to `CALL_RVMARKER`, `CALL_BTI` and basically all other pseudo call instructions in LLVM. Is adding an attribute to `MachineInstr` the preferred approach instead? > I would also suggest that you at least don't name these as "KCFI_something", > as in the future others might want to reuse the same structure for other CFI > approaches. Always happy to hear suggestions for alternative naming. Did you have something in mind? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
