[LLVM-C][OCaml] Update OCaml bindings to match LLVMMetadataKind in C API

[alan-j-hu]

Inserting a new enum constant in the middle of the enum breaks the ABI for that enum. Commit e298fc2 introduced this issue, which was revealed because the OCaml binding tests failed.

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Alan (alan-j-hu)

Changes

Inserting a new enum constant in the middle of the enum breaks the ABI for that enum. Commit e298fc2 introduced this issue, which was revealed because the OCaml binding tests failed.

---

Full diff: https://github.com/llvm/llvm-project/pull/132268.diff

1 Files Affected:

• (modified) llvm/include/llvm-c/DebugInfo.h (+1-1)

diff --git a/llvm/include/llvm-c/DebugInfo.h b/llvm/include/llvm-c/DebugInfo.h
index 9d0875a4ed8d8..63f0d4c7b367d 100644
--- a/llvm/include/llvm-c/DebugInfo.h
+++ b/llvm/include/llvm-c/DebugInfo.h
@@ -172,7 +172,6 @@ enum {
   LLVMDIEnumeratorMetadataKind,
   LLVMDIBasicTypeMetadataKind,
   LLVMDIDerivedTypeMetadataKind,
-  LLVMDISubrangeTypeMetadataKind,
   LLVMDICompositeTypeMetadataKind,
   LLVMDISubroutineTypeMetadataKind,
   LLVMDIFileMetadataKind,
@@ -196,6 +195,7 @@ enum {
   LLVMDIGenericSubrangeMetadataKind,
   LLVMDIArgListMetadataKind,
   LLVMDIAssignIDMetadataKind,
+  LLVMDISubrangeTypeMetadataKind,
 };
 typedef unsigned LLVMMetadataKind;
 

[alan-j-hu]

Also brought OCaml bindings up to date with the LLVMMetadataKind enum (there were some constants in the C enum that were missing corresponding cases in the OCaml type)

[alan-j-hu]

I'm not sure what LLVM's practices are; is it always recommended to add new enum variants to the end to avoid ABI breakage; or is it fine to add new enum constants in the middle, and I should just update the OCaml bindings to properly reflect the C API?

[nikic]

Uff, this is super broken. It looks like people are not aware of the fact that this enum must list entries in the same order as https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Metadata.def and that it is an append-only list. It looks like stuff like DIAssignID got inserted into the middle of the list :(

I think we should either have Metadata.def explicitly list the ID (like FixedMetadataKinds.def) or else create an explicit mapping operation between the C API enum and the C++ one.

[dwblaikie]

Could we move DebugInfo.h to use the .def file (I seem to recall another recent review where someone was proposing removing .def usage from a C API header because it made it easier for some tooling to process it - but keeping a single source of truth seems more important?)? That doesn't address the "append only" nature - but I guess put big scary comments about that as best we can.

[OCHyams]

It looks like stuff like DIAssignID got inserted into the middle of the list :(

Ah, DIAssignID placement is my mistake. I wasn't aware of the append-only nature of the list at all :/ Agree with @dwblaikie that a big scary comment would be useful there. Is it mentioned anywhere (in docs etc) at the moment?

[nikic]

Actually, I'm just wrong here, sorry. Looking closer, there actually is a mapping between the C enum and C++ enum:

llvm-project/llvm/lib/IR/DebugInfo.cpp

Lines 1857 to 1866 in 387f3e8

	LLVMMetadataKind LLVMGetMetadataKind(LLVMMetadataRef Metadata) {
	switch(unwrap(Metadata)->getMetadataID()) {
	#define HANDLE_METADATA_LEAF(CLASS) \
	case Metadata::CLASS##Kind: \
	return (LLVMMetadataKind)LLVM##CLASS##MetadataKind;
	#include "llvm/IR/Metadata.def"
	default:
	return (LLVMMetadataKind)LLVMGenericDINodeMetadataKind;
	}
	}

I didn't find it when searching for the constants because it's macro based. So actually everything is good here, and it's okay to add entries to Metadata.def in any order. Sorry again for the confusion!

[nikic]

With that in mind, this LGTM.

[nikic]

Maybe add a comment above this enum that new entries should always be appended instead of matching the order in Metadata.def?