Downgrade metadata size-related errors into warnings

[RyanGlScott]

Previously, llvm-pretty-bc-parser would produce a fatal error if it encountered a metadata record with an unexpected size. This proves to be extremely cumbersome in practice, however, as LLVM frequently adds new fields to metadata records, and this causes a fair bit of headaches when attempting to support newer LLVM versions.

This patch downgrades this class of errors into warnings. It does so by:

• Introducing a new ParseWarning data type that captures all types of parser-related warnings. (For now, the only type of ParseWarning is InvalidMetadataRecordSize, but we may add more in the future.)

• Adding new functions to Data.LLVM.BitCode (all of which end with *WithWarnings) that return ParseWarnings alongside the parsed Module. As such, this patch does not change any of the existing Data.LLVM.BitCode API. It is up to users to decide if they want to opt into the new API that offers ParseWarnings.

See the changes in the disasm-test and llvm-disasm test suite for examples of how to use the new API.

Fixes #248.

[RyanGlScott]

I'm not entirely sure yet if I like the approach that this PR uses, which is why I've marked this as a draft for the time being. The approach I took here was to collect parse-related warnings during parsing and then print them all at the end, which lets us avoid needing to change any parts of the public-facing API. Some alternative designs:

• Print out warnings as they arise (that way, we wouldn't need to collect them in psWarnings)
• Collect warnings, but change the Data.LLVM.BitCode API such that it returns warnings alongside the parsed Module. It would be up to users to decide whether warnings are printed or not.
• A combination of the ideas above.

Thoughts?

[kquick]

Since this is a library, I think the application should be able to decide if they want to handle the warnings in a different manner (so definitely not option 1: print as encountered).

What if we had a new API function that returned Either Error (Module, [ParseWarning]) and then made the existing API function a wrapper around that which would print the parse warnings and just return the module. That way it wouldn't break existing clients (although they might get some additional output noise) and clients could opt-in to their own warning handling via the new API function.

Also, these are warnings, but they should probably go to stderr and not stdout just in case they occur in an application piping output to something expecting to process that output.

Other than that, I like this, and thank you for grinding this axe.

[RyanGlScott]

What if we had a new API function that returned Either Error (Module, [ParseWarning]) and then made the existing API function a wrapper around that which would print the parse warnings and just return the module.

I like that idea, although it wouldn't just be a single function. There are currently four functions in Data.LLVM.BitCode that return IO (Either Error Module), so we would need a new variant of each of these functions that also return a list of ParseWarnings, giving us a grand total of eight such functions. That's a bit much, but perhaps we should just do it anyway.

[langston-barrett]

What if we had a new API function that returned Either Error (Module, [ParseWarning]) and then made the existing API function a wrapper

I think I would be surprised if I bumped the submodule, had everything still compile, and then got new content on stderr. IMO, if we are emitting "warnings", clients should explicitly decide what to do with them. Perhaps we have a helper that simply prints them to stderr, but with a different name so that clients are forced to opt into such behavior.

[langston-barrett]

it is usually not a serious problem for downstream users that llvm-pretty-bc-parser lacks newer metadata fields, since they usually do not affect the semantics of the overall LLVM bitcode.

I think this is the central question here. Who are the downstream applications, and what are their concerns? The main ones I can think of are Crux and SAW. These are tools used for verification, and their soundness is important to their users. How confident are we that the metadata doesn't change the semantics? I'm personally not confident, not having reviewed the documentation for various metadata in a long time. If any of us is confident (or is willing to do the research to become confident), then I'm aligned with this change.

[RyanGlScott]

How confident are we that the metadata doesn't change the semantics?

I don't think the question is "can the metadata change the semantics" but rather "can adding new metadata fields change the semantics". The former question can usually be answered with a "no" (LLVM even makes it possible to strip unused LLVM metadata as an optimization pass), but there are a few places where tools like SAW makes use of metadata information (e.g., for determining the field names of C structs). That being said, LLVM preserves backwards compatibility whenever it adds new fields to metadata records, so any changes to LLVM metadata are usually additive. As such, I'd deem it quite safe to ignore newly added metadata fields, as they wouldn't impact other parts of the metadata that already exist.

[langston-barrett]

Great! I'm aligned, then. That being said, I hope that downgrading this into a warning doesn't discourage projects that make use of llvm-pretty-bc-parser from putting in the effort to implement support and testing for a wide range of compilers, especially newer versions.

[RyanGlScott]

We include the repo's URL in the warning message, so my hope is that the warnings prove annoying enough that our users will be motivated to file issues anyway :)

[RyanGlScott]

Based on feedback, I've kept the implementation of the existing Data.LLVM.BitCode API functions unchanged, so they will continue not to print any warnings. In addition to these functions, I have also added new *WithWarnings functions that do return ParseWarnings, and I have left it up to users to decide how they want to print the warnings (if at all). Lastly, I've updated the test suites to make use of the *WithWarnings functions. Let me know what you think.

[langston-barrett]

I would be in favor of going ahead and deprecating the APIs that don't return warnings. IMO, callers should be explicit about not handling them. Certainly, we can give everyone a long lead time before removing them.

[kquick]

What if we had a new API function that returned Either Error (Module, [ParseWarning]) and then made the existing API function a wrapper

I think I would be surprised if I bumped the submodule, had everything still compile, and then got new content on stderr.

Actually in this case, I think you would get different content on stderr... and then your program would continue running, whereas in the previous version it would halt. That said, I agree in general that a library should not be performing unexpected IO activities, but my proposal was a compromise to avoid blissful ignorance. Since we've deprecated the old API that doesn't alert the user, I think the current approach is fine.

[langston-barrett]

Actually in this case, I think you would get different content on stderr... and then your program would continue running, whereas in the previous version it would halt.

Yes, good point 😁

In any case, I think we've settled on a good approach now that will minimize surprise for downstream packages on upgrades (without maximizing breakage) and help us reduce the library I/O!

[sauclovian-g]

Cleaning up SAW's printing is going to be enough fun without having its components generating more behind its back, so returning a list of warnings is fine by me. (Having machinery for the callers to install print handlers might be better in general, but that's extremely painful in Haskell.)

The one thing I don't entirely like is that on failure it should ideally return both the errors and any warnings. In general that tends to help avoid unpleasant surprises. However, for this particular warning and this particular situation I don't think it matters much.

[RyanGlScott]

The one thing I don't entirely like is that on failure it should ideally return both the errors and any warnings.

Yes, this is a reasonable suggestion. Initially, I was going to say that I don't see why we couldn't refactor things so that the *WithWarnings functions return IO (Either Error Module, Seq ParseWarning) instead of IO (Either Error (Module, Seq ParseWarning)). To a first approximation, this would require changing the definition of the Parse monad:

diff --git a/src/Data/LLVM/BitCode/Parse.hs b/src/Data/LLVM/BitCode/Parse.hs
index 232b0fa..bbd6689 100644
--- a/src/Data/LLVM/BitCode/Parse.hs
+++ b/src/Data/LLVM/BitCode/Parse.hs
@@ -61,7 +62,7 @@ formatContext :: [String] -> [String]
 formatContext cxt = "from:" : map ('\t' :) cxt
 
 newtype Parse a = Parse
-  { unParse :: ReaderT Env (StateT ParseState (Except Error)) a
+  { unParse :: ReaderT Env (ExceptT Error (State ParseState)) a
   } deriving ( Functor, Applicative, MonadFix
              , MonadReader Env
              , MonadState ParseState
@@ -100,11 +101,9 @@ instance MonadPlus Parse where
   {-# INLINE mplus #-}
   mplus = (<|>)
 
-runParse :: Parse a -> Either Error (a, ParseState)
+runParse :: Parse a -> (Either Error a, ParseState)
 runParse (Parse m) =
-  case runExcept (runStateT (runReaderT m emptyEnv) emptyParseState) of
-    Left err  -> Left err
-    Right res -> Right res
+  runState (runExceptT (runReaderT m emptyEnv)) emptyParseState
 
 notImplemented :: Parse a
 notImplemented  = fail "not implemented"

That way, we can access the ParseState (which contains the accumulated warnings) even if an Error is thrown.

Some further investigation reveals that this change wouldn't be enough on its own, however. Unfortunately, there are other exception-related parts of the code that this would impact. For example, consider this part of the parseBitStream function:

llvm-pretty-bc-parser/src/Data/LLVM/BitCode.hs

Lines 36 to 37 in 965d6c9

	Right bits -> X.handle (return . Left . badRefError)
	(X.evaluate (runParse (parseModule bits)))

This calls runParse in a giant exception handler to catch any BadForwardRef exceptions, which the internals of the bitcode parser can throw in pure code (example). If we changed the type signature of runParse as described above, then we would need to update the first argument to handle in order to return a Seq ParseWarning. But what would we return here? We don't have access to ParseState here, so just about the only thing we could return here is an empty sequence.

Returning an empty sequence feels off, however, as it defeats the purpose of returning warnings in the event of the failure in the first place. Really, this suggests that this use of handle is a hack, and that we should convert BadForwardRef exceptions into Errors that can be understood by the Parse monad's MonadError instance. Doing so would be a somewhat large undertaking, however, since we would have to convert all places where BadForwardRef exceptions are thrown in pure code into equivalent code in the Parse monad. This would likely be doable, but it would require lots of incidental refactoring.

All of this is to say: I wouldn't be opposed to performing these changes, especially if you think we'd end up with a more sensible API in the long run. I'd need some extra time to get there, on the other hand.

[sauclovian-g]

I would say don't chase after it for now. This is the sort of thing the general printing cleanup in saw-script is supposed to cope with, and after that sweep's done I can easily carry it into here... and at that point I'll have dealt with all these sorts of problems recently and have recommended solutions to hand.

[RyanGlScott]

I'm happy to merge this as-is (less work for me), but this does mean that if we want to print warnings in the event of failure, then we will have to change the API once again at a later date.

In any case, I'll go ahead and defer this to an issue (#286), as implementing this idea proves non-trivial.