This is an archive of the discontinued LLVM Phabricator instance.

Improve testing for the C API
ClosedPublic

Authored by deadalnix on Jun 24 2015, 10:28 PM.

Details

Summary

This basically add an echo test case in C. The support is limited right now, but full support would just be too much to review at once.

The echo test case simply get a module as input and try to output the same exact module. This allow to check the both reading and writing API are working as expected.

I want to improve this test over time to support more and more of the API, in order to improve coverage (coverage is quite poor right now).

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 28436.Jun 24 2015, 10:28 PM
deadalnix retitled this revision from to Improve testing for the C API.
deadalnix updated this object.
deadalnix edited the test plan for this revision. (Show Details)
deadalnix added reviewers: chandlerc, bogner.
deadalnix added a subscriber: Unknown Object (MLST).
chandlerc edited edge metadata.Jun 24 2015, 10:45 PM

So, I really like this as a way to ensure we have reasonable binding coverage of IR manipulation. But I'd like to hear others' opinions.

If we're going this direction, as discussed in IRC, i'd try to come up with a nice pattern so we don't have to manually CHECK everything.

I'd also like to float the idea of writing the code for this in C++, and just restricting it to interface via the C API. I think that would make the code here a lot cleaner and simpler. We can still use extern "C" to wire it into main.c. As part of that, I'd pick a naming convention and stick to it. My preference is to use C++ and use the LLVM naming convention consistently here. We can update the rest of llvm-c-test as we go.

Finally, running clang-format over it would be nice.

deadalnix updated this revision to Diff 28437.Jun 24 2015, 11:04 PM
deadalnix edited edge metadata.

Updated the test to use diff.

I'd say i wouldn't be against writting part of this in C++ . Some of the C code is really ugly, but I wanted ot have something up and running fast enough.

Ping ping ? How do we go with that ?

deadalnix updated this revision to Diff 31549.Aug 7 2015, 3:28 PM

Update to use DenseMap and make the echo thing a cpp file.

deadalnix updated this revision to Diff 31881.Aug 11 2015, 4:38 PM

Rebase, ping ping ping ?

deadalnix updated this revision to Diff 32196.Aug 14 2015, 3:29 PM

ping ping ping ?

deadalnix updated this revision to Diff 33354.Aug 27 2015, 1:33 PM

Testing is good. Let's have testing.

Hi,

I agree with you in general, but I would like a resolution and plan forward
in the C API thread before moving forward with adding testing.

Thanks :)

-eric

Well somone need to make a judgement call. Ther are people willing to do the work already and there is need. Right now all work on the C API is suspended in limbo, that's not good.

I think this is pretty clear that people want the C API, are willing to see some part of it being modified from one version to the other granted that :

  • These changes are only done when strictly necessary by change in LLVM. The ptr change for instance seems to be a reasonable reason to change it.
  • Preferably, new function have different names than existing ones, as C do not mangle signature, so changing the signature of a function mid-road can cause silent breakage.

It seems clear that core devs want to move fast (and I support that direction as well) and be able to refactor in depth, so there is no 50 ways around all of this.

I don't think anyone has anythign to add the the thread at this stage, everythign has been said, doesn't it ?

deadalnix updated this revision to Diff 33502.Aug 28 2015, 6:05 PM

Just fix a messed up comment block.

deadalnix updated this revision to Diff 34232.Sep 8 2015, 11:19 AM

Rebase, ping ?

deadalnix updated this revision to Diff 34737.Sep 14 2015, 2:49 PM

Rebase and ping.

deadalnix updated this revision to Diff 35306.Sep 21 2015, 2:30 PM

rebase ping

deadalnix updated this revision to Diff 36584.Oct 5 2015, 11:35 PM

ping ping ping ping ping ping ??!?

deadalnix updated this revision to Diff 37047.Oct 10 2015, 10:43 PM

Rebase and remove LLVMIsGlobalValue as LLVMIsAGlobalValue already exists.

deadalnix updated this revision to Diff 40243.Nov 15 2015, 6:38 PM

Rebase.

@echristo , how are things coming along on the C API policy side of things ? Can we move forward with this ?

@echristo , how are things coming along on the C API policy side of things
? Can we move forward with this ?

I sent mail to the list today. Should just be another couple of days.

Thanks!

-eric

http://reviews.llvm.org/D10725

Files:

include/llvm-c/Core.h
test/Bindings/llvm-c/echo.ll
tools/llvm-c-test/CMakeLists.txt
tools/llvm-c-test/calc.c
tools/llvm-c-test/echo.cpp
tools/llvm-c-test/llvm-c-test.h
tools/llvm-c-test/main.c
tools/llvm-c-test/metadata.c
tools/llvm-c-test/module.c

For the record this is a short week, so I'm going to wait until next
tuesday to commit the new policy. Thanks for the patience.

-eric

deadalnix updated this revision to Diff 43204.Dec 17 2015, 6:04 PM

Ok now that we have a policy for C API and that D13426 is in, it is time to come back to this.

deadalnix updated this revision to Diff 43278.Dec 18 2015, 3:32 PM

Reabse and fix conflict.

echristo accepted this revision.Dec 18 2015, 4:18 PM
echristo added a reviewer: echristo.

Seems like a good direction, I'm going to let Chandler give the final ack here.

-eric

This revision is now accepted and ready to land.Dec 18 2015, 4:18 PM
deadalnix updated this revision to Diff 43409.Dec 21 2015, 3:42 PM
deadalnix edited edge metadata.

rebase, fix conflicts

Purely stylistic stuff, only super important point is the one on the test itself.

I'm happy for Eric to finish the code style review, but I think we should get the C code here to at least be consistent with the rest of llvm-c-test, and ideally as consistent as possible with LLVM's general coding conventions.

Feel free to land whenever Eric is happy.

test/Bindings/llvm-c/echo.ll
15

The location of the TODOs seems odd to me. Maybe collect them at the top?

tools/llvm-c-test/echo.cpp
175–179

This seems a particularly awkward conditional expression... Maybe just use ifs?

286

You're relying on C99 elsewhere, please just pull ind <stdbool.h> and use normal bool syntax?

echristo requested changes to this revision.Dec 21 2015, 3:52 PM
echristo edited edge metadata.

OK, I'll do a more detailed style review. Found a couple of things.

This revision now requires changes to proceed.Dec 21 2015, 3:52 PM
deadalnix updated this revision to Diff 43433.Dec 22 2015, 12:16 AM
deadalnix edited edge metadata.

@chandlerc comments

chandlerc added inline comments.Dec 22 2015, 12:52 AM
tools/llvm-c-test/echo.cpp
2–13

Note that as this is now C++ code just using the C API, this header should be updated to be the normal C++ header...

287

Perhaps more relevantly, this is C++ code now, so you don't need stdbool.h or anything ,you can use all the glorious power of C++. ;]

deadalnix updated this revision to Diff 43543.Dec 23 2015, 9:24 AM
deadalnix edited edge metadata.

update header, use C++'s bool

deadalnix updated this revision to Diff 43865.Jan 3 2016, 2:25 PM

Rebase, add support to be able to do DenseMap of opaque types.

OK, I've started in on the inline comments. I started adding one about naming conventions on the functions, but I notice now that this just goes with the rest of the files - which is fine I guess. In the C++ code, as much as possible, do use the llvm naming/style conventions.

Thanks!

-eric

tools/llvm-c-test/calc.c
17

Commit this separately if it isn't necessary.

tools/llvm-c-test/echo.cpp
11–12

Please elaborate here on what this testing tool is doing and how it's testing things.

18–19

Shouldn't be necessary anymore?

21

Move this with the rest of the llvm includes.

26

You changed it to use a DenseMap, but I'm curious why?

77–81

This is C++ now yes?

149

Can you make the comments here more elaborate of what you're looking for here?

301–303

llvm coding style is no braces around single line blocks. (And in other places).

deadalnix added inline comments.Jan 12 2016, 5:05 PM
tools/llvm-c-test/echo.cpp
26

The default implementation require knowledge of pointer alignement. The C API pointer are opaques, so it doesn't work. The solution was suggested by @chandlerc on IRC.

77–81

Yes, but still very much C style. Do you want me to use a Vector or something alike ?

deadalnix added inline comments.Jan 13 2016, 3:59 AM
tools/llvm-c-test/calc.c
17

Core is now included in llvm-c-test.h so not needed here anymore.

deadalnix updated this revision to Diff 44735.Jan 13 2016, 4:16 AM
  • Rebase
  • Use nullptr
  • Braceless one line ifs
  • Added comments on top to explain what this does and why.
  • Change clone_instruction into clone_value as this is what it does.
  • Rework various comments.

Adding another pair of eyes to this review, spotted one thing, looks good otherwise.
(That said this is my first review of LLVM code).

include/llvm-c/Core.h
1203

This change seems completely unrelated.

deadalnix added inline comments.Jan 16 2016, 2:00 PM
include/llvm-c/Core.h
1203

Indeed, there was a typo here in the past, but it has been fixed and now this change don't make ay sense anymore, good catch.

deadalnix updated this revision to Diff 45144.Jan 17 2016, 5:16 PM

Remove comment change

mehdi_amini added inline comments.
tools/llvm-c-test/echo.cpp
33

Comment.

45

Why not just reuse hash_value?

return hash_value(PtrVal);
deadalnix added inline comments.Jan 19 2016, 7:35 PM
tools/llvm-c-test/echo.cpp
45

I just based myself on the default implementation for DenseMapInfo, I can use that if you think this is more appropriate.

deadalnix updated this revision to Diff 45346.Jan 19 2016, 9:10 PM

Add comment to explain why a custom DenseMapInfo is needed and use hash_value

Few more inline comments and can you mark the ones you've completed as Done please?

Thanks!

-eric

tools/llvm-c-test/calc.c
17

Ah, I see you're doing it in this patch. OK.

tools/llvm-c-test/echo.cpp
14

"This command uses the C API to read a module and output an exact copy of it as output. It is used to check that the resulting module matches the input to validate that the C API can read and write modules properly."

78–82

Yep :)

deadalnix updated this revision to Diff 45471.Jan 20 2016, 5:23 PM

Use SmallVector for call argument. Did not do it for function type as the API dumps it all at once in a buffer and this doesn't map well on top of C++ .

deadalnix added inline comments.Jan 20 2016, 5:27 PM
test/Bindings/llvm-c/echo.ll
16

Done

tools/llvm-c-test/echo.cpp
22

Done

34

Done

46

Done

82

OK This is not really working for that one (The API expect to dump in a buffer, and the C++y code is worse than straight C) but I did it for call instructions.

150

I think naming the function correctly makes it clear. Also added some comment withing the function to explain what is going on.

180

Just used ifs :)

288

Used true/false and nullptr all over.

304

Removed braces around one liner.

deadalnix updated this revision to Diff 45472.Jan 20 2016, 5:28 PM

Remove unused include

This is starting to shape up nicely. I've got a few more inline nits, and one generic one which is "Could you please document the functions?" Basically document what the arguments and return values are producing and used for.

Thanks!

-eric

tools/llvm-c-test/echo.cpp
83

Can you try to explain a different way? I'm not sure what you mean.

151

Then you should probably add asserts. Also, the coding convention is complete sentences for comments.

Something as simple as "Try to clone everything in the llvm::Value hierarchy." is a start on commenting the function.

219

Go ahead and remove the "add a new binding" todos as they're not relevant here.

228–230

No braces necessary for a single statement.

271

Should probably have a clone_instruction separated out from clone_value so that anything weird is handled?

284

Can you make this an error that's clear about what is going on? Ditto with the rest of them in this function, clone_bbs, etc.

335–338

Can you add a test verifying this is working to your test?

403

"iteration"

deadalnix added inline comments.Jan 21 2016, 9:23 AM
tools/llvm-c-test/echo.cpp
83

Sure. The trick part is that LLVMGetParamTypes require a buffer passed down as a pointer and will fill it all. So we need to create a buffer and fill it. Vector like API do not really lend itself to this.

dblaikie added inline comments.
tools/llvm-c-test/echo.cpp
83

It's pretty common to use std::vector when interacting with C APIs from C++ - not sure if we're picturing the same thing. This is what I'd imagine:

std::vector<LLVMTypeRef> Params(ParamCount);
LLVMGetParamTypes(Src, Params.data());
... (probably use Params directly without using ParamCount at this point)
... (skip the "if ParamCount / free" part, because it'll be cleaned up naturally)

Is there something about that that doesn't work/did you have something else in mind, etc?

deadalnix added inline comments.Jan 21 2016, 3:18 PM
tools/llvm-c-test/echo.cpp
83

Yeah I can do that, but at the end, the Vector is fed to the C API as pointer + length, so none of the feature of vector are use or are usable.

deadalnix added inline comments.Jan 21 2016, 5:34 PM
tools/llvm-c-test/echo.cpp
271

Can we defers that to a subsequent diff ? Changing this in that one will end up in rebase hell on my side as I have prepared various extension on that diff.

284

What wording would you suggest ? I have hard time coming up with something much better. This just check that when you do one step forward and one step backward, you end up where you came from.

335–338

Well if the code ever go through this codepath, that means there is a bug in the C API. I'm not sure how I can trigger it.

deadalnix updated this revision to Diff 45629.Jan 21 2016, 5:35 PM
  • Removed TODOs
  • Remove braces for loop
  • Fix typos
deadalnix updated this revision to Diff 46567.Feb 1 2016, 12:24 PM

ping ?!??!

echristo accepted this revision.Feb 4 2016, 2:41 PM
echristo edited edge metadata.

This is looking pretty good. Definitely for a first start. Thanks for doing this and sticking with it.

-eric

tools/llvm-c-test/echo.cpp
272

Sure.

285

Nothing is coming to mind at the moment, we can just defer it.

336–339

I think I was just wanting a testcase with more than zero parameters when I asked.

This revision is now accepted and ready to land.Feb 4 2016, 2:41 PM
deadalnix added inline comments.Feb 4 2016, 3:29 PM
tools/llvm-c-test/echo.cpp
336–339

I have more coming :)

This revision was automatically updated to reflect the committed changes.