[bindings] Add OCaml binding to `LLVMGlobalSetMetadata` #131583

lynzrand · 2025-03-17T09:08:22Z

This PR adds the OCaml binding to LLVMGlobalSetMetadata, so that OCaml users can set metadata on global values like functions. This is useful for e.g. adding debug information to functions.

(Note that the existing set_metadata function operates on instructions, and cannot be used to set metadata on global values.)

This is my first PR to LLVM, so please tell me if I have done anything wrong.

Although the contribution guide said that contributions should include a unit test, I can't find any unit test in the OCaml bindings, so I guess I can skip that? My fault, I didn't see the test directory. I will add tests soon. Tests are added now.

This binding has been tested in our internal projects and worked.

The code within this PR came from a branch of version 18.x. As far as I know, related C bindings haven't changed during the times, so this patch should still be relevant.

github-actions · 2025-03-17T09:08:44Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

github-actions · 2025-03-17T15:53:05Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 6b47bba44087caa7d4805bdb3229153a3bfba7a5 0bf3e74a492894f7fb10227797d5d7df73001b08 --extensions c -- llvm/bindings/ocaml/llvm/llvm_ocaml.c

View the diff from clang-format here.

diff --git a/llvm/bindings/ocaml/llvm/llvm_ocaml.c b/llvm/bindings/ocaml/llvm/llvm_ocaml.c
index 7d32b8b2f6..e3960bac31 100644
--- a/llvm/bindings/ocaml/llvm/llvm_ocaml.c
+++ b/llvm/bindings/ocaml/llvm/llvm_ocaml.c
@@ -1547,7 +1547,8 @@ value llvm_set_global_constant(value Flag, value GlobalVar) {
 }
 
 /* llvalue -> llmdkind -> llmetadata -> unit */
-value llvm_global_set_metadata(value Value, value MetadataKind, value Metadata) {
+value llvm_global_set_metadata(value Value, value MetadataKind,
+                               value Metadata) {
   LLVMGlobalSetMetadata(Value_val(Value), (unsigned int)Int_val(MetadataKind),
                         Metadata_val(Metadata));
   return Val_unit;

Format code according to formatter requirements

lynzrand · 2025-03-18T01:58:52Z

BTW, what is the default OCamlFormat parameters for the binding files? I have OCamlFormat installed locally, but it seems like it's formatting using a different config than the current one. I'm currently maintaining format of OCaml files by hand.

alan-j-hu · 2025-03-20T03:14:22Z

Taking a look at this. Right now I'm getting some test failures involving debuginfo.ml, but I'm still trying to figure out if thy were caused by this PR or some other issue on my end, etc.

alan-j-hu · 2025-03-20T03:20:50Z

When you run the unit tests, is the OCaml debuginfo test failing for you, or is this only on my end?

lynzrand · 2025-03-20T03:28:42Z

I'm getting the debuginfo.ml failures too.

I'm not sure what's happening here -- I also get failures on the main branch, although with a different error. I haven't even touched any existing interfaces at all.

The one I get in this branch is:

RUN: at line 3: llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/executable | llvm-project/build/bin/FileCheck lvm-project/llvm/test/Bindings/OCaml/debuginfo.ml
+ llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/executable
+ llvm-project/build/bin/FileCheck /home/rynco/Projects/llvm/llvm-project/llvm/test/Bindings/OCaml/debuginfo.ml
FAILED: //module_level_tests #5
FAILED: //module_level_tests #6
FAILED: //module_level_tests #7
FAILED: //function_level_tests #1
FAILED: //function_level_tests #3
FAILED: //basic_block and instructions tests #4
FAILED: //global variable expression tests #3
FAILED: //type tests #1
FAILED: //type tests #4
FAILED: //type tests #11

And the one I get on main branch is this, which is even weirder. But maybe because I have a different version of LLVM opam package installed:

RUN: at line 1: rm -rf llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp && mkdir -p llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp && cp llvm-project/llvm/test/Bindings/OCaml/debuginfo.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml && cp llvm-project/llvm/test/Bindings/OCaml/Utils/Testsuite.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml
+ rm -rf llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp
+ mkdir -p llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp
+ cp llvm-project/llvm/test/Bindings/OCaml/debuginfo.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml
+ cp llvm-project/llvm/test/Bindings/OCaml/Utils/Testsuite.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml
RUN: at line 2: ocamlfind ocamlc -cclib -Lllvm-project/build/lib  -g -w +A -package llvm.all_backends -package llvm.target -package llvm.analysis -package llvm.debuginfo -I llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/ -linkpkg llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml -o llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/executable
+ ocamlfind ocamlc -cclib -Lllvm-project/build/lib -g -w +A -package llvm.all_backends -package llvm.target -package llvm.analysis -package llvm.debuginfo -I llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/ -linkpkg llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml -o llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/executable
findlib: [WARNING] Package llvm has multiple definitions in llvm-project/build/lib/ocaml/META.llvm, <...>/lib/llvm/META
File "llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/Testsuite.ml", line 1:
Warning 70 [missing-mli]: Cannot find interface file.
File "llvm-project/build/test/Bindings/OCaml/Output/debuginfo.ml.tmp/debuginfo.ml", line 1:
Error: The files llvm-project/build/lib/ocaml/llvm/llvm.cmi
       and llvm-project/build/lib/ocaml/llvm/llvm_debuginfo.cmi
       make inconsistent assumptions over interface Llvm

alan-j-hu · 2025-03-20T03:51:46Z

I frequently get this error as well:

Error: The files llvm-project/build/lib/ocaml/llvm/llvm.cmi
       and llvm-project/build/lib/ocaml/llvm/llvm_debuginfo.cmi
       make inconsistent assumptions over interface Llvm

IIRC it's a bug in some CMakeLists.txt file. You need to explicitly run ninja -Cbuild ocaml_llvm_debuginfo again.

lynzrand · 2025-03-20T06:09:54Z

Reran. Seems like the same debuginfo test failure is in main branch too, so it's not caused by this PR.

alan-j-hu · 2025-03-20T14:25:48Z

Looks like commit 6b47bba, the last commit on main, has the debuginfo test failure.

Commit d8b2e43, the last commit to directly touch the OCaml bindings, does not have the debuginfo test failure.

Commit a5fb2bb, the last commit to directly touch the OCaml debuginfo bindings, does not have the debuginfo test failure.

So, it looks like the debuginfo test failure was introduced somewhere in-between commit d8b2e43 and commit 6b47bba, in code that did not touch the OCaml bindings.

alan-j-hu · 2025-03-20T17:53:50Z

I used git bisect, which determined that the breaking commit was 8dbc393. This commit seems to be unrelated, so I need to figure out why it breaks the debuginfo test.

EDIT: I ran the test again and it seems to be fine, so now I need to do more investigation. Did I mess up the git bisect somewhere?

alan-j-hu · 2025-03-20T17:58:03Z

Looks like the bad commit was the one right after; e298fc2.

alan-j-hu · 2025-03-20T19:42:25Z

Okay, I verified that the failing test doesn't have to do with this PR.

BTW, what is the default OCamlFormat parameters for the binding files? I have OCamlFormat installed locally, but it seems like it's formatting using a different config than the current one. I'm currently maintaining format of OCaml files by hand.

I don't believe the bindings use an ocamlformat file. There is some kind of consistent style that you should try to adhere to, but they're not being enforced by any formatting rules.

lynzrand · 2025-03-21T02:11:35Z

Okay, I verified that the failing test doesn't have to do with this PR.

Thanks!

BTW, what is the default OCamlFormat parameters for the binding files? I have OCamlFormat installed locally, but it seems like it's formatting using a different config than the current one. I'm currently maintaining format of OCaml files by hand.

I don't believe the bindings use an ocamlformat file. There is some kind of consistent style that you should try to adhere to, but they're not being enforced by any formatting rules.

I was asking this because I saw the two .ocamlformat files in the binding and test directories. Doing some git log checking, seems like they were added in 506df1b and c244cd7, and remained empty since then. It doesn't seem like any of the OCaml files are using the formatter.

IMHO, although unrelated to this PR, that if we have the .ocamlformat files, maybe we should consider using them to enforce a specific formatting of the sources? Or if we aren't using them anyway, we might consider deleting them (in a later PR)?

llvm/test/Bindings/OCaml/core.ml

llvm/bindings/ocaml/llvm/llvm.ml

Fix typo and formats Co-authored-by: Alan <[email protected]>

alan-j-hu

I noticed that you made the metadata tests "looser" by checking for a numeric regex instead of the precise metadata ID. As far as I can tell, the tests are still deterministic. Would it be better to continue checking for the exact metadata ID, as the file currently does?

alan-j-hu · 2025-03-21T03:01:31Z

llvm/test/Bindings/OCaml/core.ml

+ * CHECK: !llvm.module.flags = !{!{{[0-9]+}}}
+ * CHECK: !{{[0-9]+}} = !{i32 0, !"global test metadata"}
+ * CHECK: !{{[0-9]+}} = !{i32 1, !"Debug Info Version", i32 3}
+ * CHECK: !{{[0-9]+}} = !{i32 1, !"metadata test"}


Suggested change

* CHECK: !llvm.module.flags = !{!{{[0-9]+}}}

* CHECK: !{{[0-9]+}} = !{i32 0, !"global test metadata"}

* CHECK: !{{[0-9]+}} = !{i32 1, !"Debug Info Version", i32 3}

* CHECK: !{{[0-9]+}} = !{i32 1, !"metadata test"}

* CHECK: !llvm.module.flags = !{!{{[0-9]+}}}

* CHECK: !0 = !{i32 0, !"global test metadata"}

* CHECK: !1 = !{i32 1, !"Debug Info Version", i32 3}

* CHECK: !2 = !{i32 1, !"metadata test"}

alan-j-hu · 2025-03-21T03:02:31Z

llvm/test/Bindings/OCaml/core.ml

+    (* CHECK: %metadata = add i32 %P1, %P2, !test !{{[0-9]+}}
+     * Number of metadata nodes is not predictable, so we just check for
+     * the presence of metadata here


Even if you were to prefer to check for a numeric regex instead of a precise ID, I don't like the wording of "predictable" in the comment. The tests are still deterministic, i.e. predictable.

Suggested change

(* CHECK: %metadata = add i32 %P1, %P2, !test !{{[0-9]+}}

* Number of metadata nodes is not predictable, so we just check for

* the presence of metadata here

(* CHECK: %metadata = add i32 %P1, %P2, !test !2

lynzrand · 2025-03-21T09:05:55Z

I see what you mean here. I will change it back to absolute numbers.

Change test metadata IDs back to numbers

Forgot one place to revert

The module flags is wrong

alan-j-hu · 2025-03-21T16:35:00Z

I approve these changes, and will squash and merge them. For the commit message, I will just use the message for the first commit - [bindings] Add `global_set_metadata` for function debuginfo. Is this fine?

lynzrand · 2025-03-22T01:06:50Z

Yes, thank you!

…

________________________________ From: Alan ***@***.***> Sent: Saturday, March 22, 2025 12:35:30 AM To: llvm/llvm-project ***@***.***> Cc: Rynco Maekawa ***@***.***>; Author ***@***.***> Subject: Re: [llvm/llvm-project] [bindings] Add OCaml binding to `LLVMGlobalSetMetadata` (PR #131583) I approve these changes, and will squash and merge them. For the commit message, I will just use the message for the first commit - [bindings] Add `global_set_metadata` for function debuginfo. Is this fine? — Reply to this email directly, view it on GitHub<#131583 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACOIVL6H2ENGWKQFOOKSX6D2VQ5VFAVCNFSM6AAAAABZE5ZN5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBTHA4TEMRSHA>. You are receiving this because you authored the thread.Message ID: ***@***.***> [alan-j-hu]alan-j-hu left a comment (llvm/llvm-project#131583)<#131583 (comment)> I approve these changes, and will squash and merge them. For the commit message, I will just use the message for the first commit - [bindings] Add `global_set_metadata` for function debuginfo. Is this fine? — Reply to this email directly, view it on GitHub<#131583 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACOIVL6H2ENGWKQFOOKSX6D2VQ5VFAVCNFSM6AAAAABZE5ZN5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBTHA4TEMRSHA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

github-actions · 2025-03-22T20:36:56Z

@lynzrand Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[bindings] Add global_set_metadata for function debuginfo

0bf3e74

lynzrand marked this pull request as ready for review March 17, 2025 09:14

nikic requested a review from alan-j-hu March 17, 2025 15:49

fixup! [bindings] Add global_set_metadata

2d4484f

Format code according to formatter requirements

[bindings][test] Add test for global_set_metadata

4b1c337

alan-j-hu reviewed Mar 21, 2025

View reviewed changes

llvm/test/Bindings/OCaml/core.ml Outdated Show resolved Hide resolved

alan-j-hu reviewed Mar 21, 2025

View reviewed changes

llvm/bindings/ocaml/llvm/llvm.ml Outdated Show resolved Hide resolved

fixup! [bindings][test] Add test for global_set_metadata

75b272b

Fix typo and formats Co-authored-by: Alan <[email protected]>

alan-j-hu reviewed Mar 21, 2025

View reviewed changes

lynzrand added 3 commits March 21, 2025 17:23

fixup! [bindings][test] Add test for global_set_metadata

ae10076

Change test metadata IDs back to numbers

fixup! [bindings][test] Add test for global_set_metadata

86429bc

Forgot one place to revert

fixup! [bindings][test] Add test for

7dcbc3d

The module flags is wrong

alan-j-hu approved these changes Mar 21, 2025

View reviewed changes

alan-j-hu merged commit 5d54043 into llvm:main Mar 22, 2025
7 checks passed

[bindings] Add OCaml binding to LLVMGlobalSetMetadata #131583

[bindings] Add OCaml binding to LLVMGlobalSetMetadata #131583

Uh oh!

Conversation

lynzrand commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 17, 2025

Uh oh!

github-actions bot commented Mar 17, 2025

Uh oh!

lynzrand commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-j-hu commented Mar 20, 2025

Uh oh!

alan-j-hu commented Mar 20, 2025

Uh oh!

lynzrand commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-j-hu commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lynzrand commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-j-hu commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-j-hu commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-j-hu commented Mar 20, 2025

Uh oh!

alan-j-hu commented Mar 20, 2025

Uh oh!

lynzrand commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

alan-j-hu left a comment

Choose a reason for hiding this comment

Uh oh!

alan-j-hu Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-j-hu Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynzrand commented Mar 21, 2025

Uh oh!

alan-j-hu commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lynzrand commented Mar 22, 2025 via email

Uh oh!

Uh oh!

github-actions bot commented Mar 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[bindings] Add OCaml binding to `LLVMGlobalSetMetadata` #131583

[bindings] Add OCaml binding to `LLVMGlobalSetMetadata` #131583

lynzrand commented Mar 17, 2025 •

edited

Loading

lynzrand commented Mar 18, 2025 •

edited

Loading

lynzrand commented Mar 20, 2025 •

edited

Loading

alan-j-hu commented Mar 20, 2025 •

edited

Loading

lynzrand commented Mar 20, 2025 •

edited

Loading

alan-j-hu commented Mar 20, 2025 •

edited

Loading

alan-j-hu commented Mar 20, 2025 •

edited

Loading

alan-j-hu Mar 21, 2025 •

edited

Loading

alan-j-hu Mar 21, 2025 •

edited

Loading

alan-j-hu commented Mar 21, 2025 •

edited

Loading