Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support redis versioning #424

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support redis versioning #424

wants to merge 1 commit into from

Conversation

pranavk
Copy link
Contributor

@pranavk pranavk commented Jan 26, 2019

Stores a new key "version" in redis db which contains the git hash
souper was built with (or timestamp if git hash is unavailable). If git
hash of souper matches value of "version" in redis db during connection,
all is good. If it doesn't matches, we error out. If no "version" key
matches, we put current souper version in the redis database and warn
the user that results might be incorrect.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/KVStore/KVStore.cpp Outdated Show resolved Hide resolved
lib/KVStore/KVStore.cpp Outdated Show resolved Hide resolved
lib/KVStore/KVStore.cpp Outdated Show resolved Hide resolved
lib/KVStore/KVStore.cpp Show resolved Hide resolved
@regehr
Copy link
Collaborator

regehr commented Jan 28, 2019

please add:

  • a test case for redis versioning
  • when souper is in debug mode (DebugLevel > 1 perhaps) print the database version and the version of the running code

@pranavk
Copy link
Contributor Author

pranavk commented Jan 31, 2019

note/TODO to self: just noticed that ninja is not happy with this cmake file while make reports no problems.

@pranavk
Copy link
Contributor Author

pranavk commented Feb 12, 2019

okay. above TODO is fixed now. It works with ninja now. I think I tried this approach earlier also but it was stymied by the pthread bug that I pushed a fix for above. This time I was careful and tracked it down; so, one shot, two bugs ;)

@regehr
Copy link
Collaborator

regehr commented Feb 12, 2019

ok, is this ready to merge? it looks fine to me. you've tested it out I assume?

if all is ready, please squash

@pranavk
Copy link
Contributor Author

pranavk commented Feb 12, 2019

i need to add a couple of things you mentioned above (redis tests, etc.). Will let you know when done.

@regehr
Copy link
Collaborator

regehr commented Feb 20, 2019

what's status of this pr?

@pranavk
Copy link
Contributor Author

pranavk commented Feb 20, 2019

this turned out to be much more complicated than i anticipated. It works on my desktop computer and fails intermittently on my laptop. There are some CI failures as well where it doesn't work. Turns out i need to give it another look (i am just avoiding this problem for now to have a look at it from fresh perspective in coming weeks).

the problem remaining is the same: ninja doesn't like the way i am doing header generation in cmake and compiles the C++ file, which depend on the generated header, before generating the header.

@regehr
Copy link
Collaborator

regehr commented Feb 20, 2019

ok thanks
no big rush on this

@pranavk pranavk changed the title WIP: support redis versioning Support redis versioning Apr 20, 2019
@pranavk
Copy link
Contributor Author

pranavk commented Apr 20, 2019

Okay. So I changed this and now we set the new key in the redis database equal to the "argument names" passed to the command line. Eg: if you invoke souper with following command line:

./souper -z3-path=/usr/bin/z3 -solver-timeout=15 -souper-external-cache -souper-debug-level=3 a.ll

we will construct the following key out of it (let's call it A).
solver-timeout,souper-external-cache,z3-path, (yes, this will be sorted and debug-levels are excluded).

and then we will check if key named cachetype exist in the redis db or not. If it doesn't exist, we insert a new key called cachetype and set its value to A. If it does exist and is not equal to A, we error out and don't proceed. And obviously if it does exist and is equal to A, we continue like normal.

Note that A doesn't contain the "values" of the command line (things after "=") -- there's just no way you can get these values from LLVM's CommandLine library with current API. I am hoping that this could be sufficient for our use-case. After discussion with @zhengyangl , the dataflow stuff is itself a command line options; so, if you already have a cache tagged with say infer-known-bits,infer-power-two, you can't use the same cache when command lines arguments doesn't involve these.

Let me know if this works.

@pranavk pranavk force-pushed the redis branch 2 times, most recently from 8eb1626 to 39435b8 Compare April 22, 2019 05:57
default: return false;
}

return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is dead code, please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a dead code.

  1. note that REDIS_REPLY_NIL and REDIS_REPLY_STRING don't return anything when things are compatible --- they just break out of the switch case and let this statement return true.
  2. Even if I put return statements to return true as described in (1), compiler will emit warning saying that control reaches end of non-void statement as we don't disable Wreturn-type from our CMake config (which is fine).

@regehr
Copy link
Collaborator

regehr commented Apr 23, 2019

there's just no way you can get these values from LLVM's CommandLine library with current API
how sure are you about this?

@pranavk
Copy link
Contributor Author

pranavk commented Apr 23, 2019

Pretty sure. The CommandLine library exposes getRegisteredCommands that give us access to the Option *. Now the problem is that values for option is part of the cl::opt class which is a subclass of Option class. In a normal situation, I would do a dynamic_cast and get a cl::opt * but in this case, I can't do that because LLVM doesn't use RTTI. My other option is to use LLVM's dyn_cast but I can't use that as well because in order to be able to use dyn_cast without RTTI, classes needs to have a method called classof in their body but these classes doesn't have that.

@regehr
Copy link
Collaborator

regehr commented Apr 23, 2019

so you're saying you can't do the getValue() thing suggested here?
https://stackoverflow.com/questions/43336602/where-to-find-o-output-argument-in-clang-llvm

@regehr
Copy link
Collaborator

regehr commented Apr 23, 2019

if it's not practical to make that work, I think you'll need to make a list of options that have interesting stuff on the RHS and look for those specifically. like the number of synthesis components, this totally matters, that sort of thing is the whole reason for the cache tags, it really doesn't make sense to do this and leave those out.

@pranavk
Copy link
Contributor Author

pranavk commented Apr 23, 2019

cl::opt is a template class with 3 parameters. So for any given type T, there are many possible instances of the class. I can't do static_cast as suggested in the link you pasted because I don't know what template class a given option is going to use unless I bloat the code and put all this information in a huge function (which doesn't sound like a good solution)

@pranavk
Copy link
Contributor Author

pranavk commented Apr 23, 2019

like the number of synthesis components, this totally matters,

yeah, that's true. I will think of something. Since it's clear now that the current patch is not sufficient, I will think more about it

@pranavk
Copy link
Contributor Author

pranavk commented May 14, 2019

like the number of synthesis components, this totally matters,

as we discussed in the last meeting, i went ahead with whitelisting some of the synthesis options. Now it includes values of the options a well. It should be easy whitelist new options.

@regehr
Copy link
Collaborator

regehr commented Dec 8, 2019

what's the status of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants