[debug] Add manticore::debug

[kesyog]

Started with PR #108 and made some non-structural changes to get it working:

• Rename trace! to fail! to avoid naming conflict with log::trace.
• Fix some minor syntax errors to get the macros to compile and work properly.

A couple of things I wasn't totally sure about:

1. Am I correct that the advantage of the redactable log macros (info!, error!, etc.) over the base log crate is that the log crate still has some overhead ("an integer load, comparison and jump") even if no logger is set up?
2. @mcy said in his original PR that the switch to the new Result type might not be able to be done gradually. Any particular reason why that would be?

[mcy]

Am I correct that the advantage of the redactable log macros (info!, error!, etc.) over the base log crate is that the log crate still has some overhead ("an integer load, comparison and jump") even if no logger is set up?

It's not the overhead, but rather being able to compile out the log format messages altogether; it is useful for us to be able to say "if you flip this flag, formatting is redacted at the linker level".

[mcy]

This is awesome! Now, the question is... do you want to attempt to migrate the entire crate over to manticore::Result? Unfortunately I think that needs to be done in one shot. =/

We can merge this and do it in a separate CL, or you can give it a shot in this one. I suspect the way you'll wind up doing this is:

• Replace Result with crate::Result throughout.
• Add fail, check and cast where necessary until the crate compiles again.

If you can think of a way to do this gradually that would be even more awesome, because then you won't be rebasing over everyone else and I can help you get it done faster.

[kesyog]

This first macro rule is an ugly hack so that this macro can be used with Error<Header> in src/server/handler.rs. See usage. It works... but I'm not too proud of it. I'm super new to Rust macros, but based on what I've read, there's not really a clean way of doing this besides resorting to proc macros, which seems like a rabbit hole not worth tackling yet. That said, feedback would be appreciated if you think otherwise.

[mcy]

Not really. One option to cut down on repetition is to define something like

macro_rules! debug_from {
  (@actual $e:ident[$($tparams:tt)*] => ...) => {
    // Use `$($tparams)* as appropriate in <>
  };
  ($e:ident => ...) => {debug_from!(@actual $e[] => ...)};
}

Note that wherever a <> is allowed, you can leave it empty, e.g. i32<> is valid Rust. You can use this to your advantage here.

[kesyog]

I bashed my head against this a bit, but I'm struggling to get any sort of recursive calls to work as I'd expect. For now, I combined two of the macro arms by matching on the tt type.

[mcy]

Sg. We can bash on it more later.

[kesyog]

Hey @mcy, while looking into using the new manticore::Result type everywhere, I think I found a way to switch over to the new fail! and check! macros somewhat incrementally. Check out the latest commit.

The approach is to add a few more From implementations to debug_from! so that the ? Try operator still works everywhere. In order to get it to compile again, I still need to fix a bunch of instances where Err was being returned directly, but otherwise I think it should work.

Let me know what you think. I've moved this PR back to draft status until I fix all the compiler errors.

[kesyog]

I pushed another commit that fixes most of the compiler errors. I highly recommend viewing the individual commits. Assuming this approach is acceptable, the next steps I'm envisioning are:

To do in this PR

• Figure out an appropriate Result boundary between the x509 modules and the untrusted crate. That crate leaks in a core::result::Result type and it might make sense to simply use that type to reduce the amount of error mapping.
• Fix remaining few compiler errors
• Double-check changes, especially wherever the check! was added, to make sure that logical errors weren't introduced.
• Run and fix unit tests if necessary

Future PR(s)

• Incrementally add error messages to the fail! macros
• Incrementally replace usage of the ? operator with the fail! macro so that error messages can be introduced.
• (maybe) Add filename and line number to the default fail! output this is already provided by the logger
• (maybe) Replace debug_from! macro with a proc macro. Based on what I've gathered, a proc macro is the way to get around the parsing limitations of declarative macros and more robustly support error types with generic parameters. I'm at the edge of my current level of experience here so I've got no idea how much work this is and whether it's worth doing.

[kesyog]

After rebasing, I noticed that #148 added a new Error in protocol/spdm/error.rs that has a lifetime parameter to refer to some extra error data. I have to think a bit more on how to handle this. My initial thought is that I'd have to add a lifetime parameter to manticore::Error unless I threw away the extra error data or copied it.

[kesyog]

Some updates:

• I added some more hackery to debug_from! to handle error types with lifetime specifiers
• I drew a boundary at the cert/mod.rs level. The parse interface returns the new crate::Result type, but the underlying parsers use the old Result type. This reduces churn and minimizes conflict with the untrusted crate's return types.
• cargo build succeeds and tests pass! 🎉

[kesyog]

@mcy this is now ready for review and potential merge. I've held off on squashing in case it helps review, but I'll make sure to squash all the commits prefixed with "SQUASHME." I recommend reading my stream of thoughts above for full context.

[mcy]

This is great work. There's a few changes I might suggest but I think those would be better left as follow-ups.

[mcy]

Not really. One option to cut down on repetition is to define something like

macro_rules! debug_from {
  (@actual $e:ident[$($tparams:tt)*] => ...) => {
    // Use `$($tparams)* as appropriate in <>
  };
  ($e:ident => ...) => {debug_from!(@actual $e[] => ...)};
}

Note that wherever a <> is allowed, you can leave it empty, e.g. i32<> is valid Rust. You can use this to your advantage here.

[mcy]

Personally I actually don't think this is a very useful error: getting an error location from a failed allocation will point inside of arena code, when it would be much more useful for the caller to see the arena call that emitted the error. WDYT?

[kesyog]

A follow-on task I'm envisioning (and that you incepted me with a few months ago 😄) is to steal ideas from rust-lang/rust#87401: investigate whether we can use #[track_caller] to automatically track the locations of error conversions performed via ?. That'd give extra, more useful debug information and would save the need to add fail! calls in many places.

So my thinking is:

1. Use the Error type with the Arena trait i.e. no change to current PR state.
2. If the debug information printed is not useful, callers can still manually add debug info. This is actually already done in some places in src/io/read.rs:

           let out = arena
               .alloc_raw(layout)
               .map_err(|_| fail!(io::Error::BufferExhausted))?;

   Since adding the fallible Arena trait calls are only made Since the Arena trait
3. Add error conversion tracking using #[track_caller] if possible. If it isn't, rely on step 2.

FWIW switching the Arena trait to using core::result::Result would be pretty easy and I can do that if you do feel strongly that seeing where in arena.rs the error originated would never be useful.

[mcy]

I don't feel strongly, if you plan on doing stuff with track_caller.

[kesyog]

I've squashed my commits together and this should be good to go now

[mcy]

Sg. We can bash on it more later.