Page MenuHomePhabricator

[UnknownProvenance] Introduce UnknownProvenance constant
AcceptedPublic

Authored by jeroen.dobbelaere on Oct 5 2021, 8:04 AM.

Details

Summary

The UnknownProvenance indicates that the provenance can be any object. This must only be used in the ptr_provenance path of a pointer.

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Oct 5 2021, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 8:04 AM
jeroen.dobbelaere updated this revision to Diff 378677.EditedOct 11 2021, 8:22 AM
  • Small cleanup of useless comment
  • NOTE: The LangeRef change explaining the purpose of unknown_provenance is done in D104268
deadalnix requested changes to this revision.Oct 12 2021, 3:36 PM
deadalnix added inline comments.
llvm/include/llvm-c/Core.h
1942

Where is the implementation for these?

llvm/include/llvm/IR/Constants.h
580

Isn't inline implied by the fact this is defined directly within the class?

This revision now requires changes to proceed.Oct 12 2021, 3:36 PM

Thanks for the feedback !

llvm/include/llvm-c/Core.h
1942

Ah.. The implementation is in D111162, but I had to move the header earlier for the enum. Probably better to just add the implementation in this revision..

llvm/include/llvm/IR/Constants.h
580

Yes it is. That's what I get for copy-pasting from ConstantPointerNull..

deadalnix added inline comments.Oct 13 2021, 3:13 AM
llvm/include/llvm-c/Core.h
1942

I'm not sure what the policy is here, but I would think that we should avoid adding stuff in the header when using them will only lead to a linker error.

But in this case, it seems that you have all the pieces to add this, at least the llvm/lib/IR/Core.cpp part.

llvm/include/llvm/IR/Constants.h
580

I think it'd be better to just remove it, as to not spread erroneous patterns (inline is notoriously misunderstood by many C/C++ devs). But either way, this is certainly not worth blocking that patch for this.

jeroen.dobbelaere marked 4 inline comments as done.Oct 13 2021, 11:47 AM

I added a llvm-c echo test in D111751, together with the ptr_provenance support.

deadalnix accepted this revision.Oct 14 2021, 6:42 AM
This revision is now accepted and ready to land.Oct 14 2021, 6:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 6:57 AM

Do you need help merging this?

Do you need help merging this?

I would like to have all patches reviewed and accepted before I start merging them into the main tree.