Page MenuHomePhabricator

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).

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 ↗(On Diff #46567)

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

tools/llvm-c-test/echo.cpp
175–179 ↗(On Diff #46567)

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

286 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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

18–19 ↗(On Diff #46567)

Shouldn't be necessary anymore?

21 ↗(On Diff #46567)

Move this with the rest of the llvm includes.

26 ↗(On Diff #46567)

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

77–81 ↗(On Diff #46567)

This is C++ now yes?

149 ↗(On Diff #46567)

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

301–303 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #44735)

This change seems completely unrelated.

deadalnix added inline comments.Jan 16 2016, 2:00 PM
include/llvm-c/Core.h
1203 ↗(On Diff #44735)

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 ↗(On Diff #46567)

Comment.

45 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

"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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

Done

tools/llvm-c-test/echo.cpp
22 ↗(On Diff #46567)

Done

34 ↗(On Diff #46567)

Done

46 ↗(On Diff #46567)

Done

82 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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

180 ↗(On Diff #46567)

Just used ifs :)

288 ↗(On Diff #46567)

Used true/false and nullptr all over.

304 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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

151 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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

228–230 ↗(On Diff #46567)

No braces necessary for a single statement.

271 ↗(On Diff #46567)

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

284 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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

403 ↗(On Diff #46567)

"iteration"

deadalnix added inline comments.Jan 21 2016, 9:23 AM
tools/llvm-c-test/echo.cpp
83 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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 ↗(On Diff #46567)

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
271 ↗(On Diff #46567)

Sure.

284 ↗(On Diff #46567)

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

335–338 ↗(On Diff #46567)

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
335–338 ↗(On Diff #46567)

I have more coming :)

This revision was automatically updated to reflect the committed changes.