Cocotb: Remove Python from gpilog

[ktbarrett]

The Python logger implementation for the GPI library is located in the gpilog library. This means that users of the GPI that aren't going to use Python still must link against libpython (this dependency on libpython isn't listed in cocotb_build_libs.py, but it is a reality). Instead, to support non-Python GPI users we should change the gpilog interface to one where the implementation is registered instead of prescribed.

Desired interface for gpilog:

/**
 * Named log levels.
 * Log levels can be any value, but only these values will be named when using the native logger.
 */
enum gpi_log_levels {
    GPIDebug = 10,
    GPIInfo = 20,
    GPIWarning = 30,
    GPIError = 40,
    GPICritical = 50
};

/**
 * Convenience functions for logging, see #gpi_log for more detail.
 */
#define LOG_DEBUG(...)     gpi_log("cocotb.gpi", GPIDebug,         __FILE__, __func__, __LINE__, __VA_ARGS__);
#define LOG_INFO(...)      gpi_log("cocotb.gpi", GPIInfo,          __FILE__, __func__, __LINE__, __VA_ARGS__);
#define LOG_WARN(...)      gpi_log("cocotb.gpi", GPIWarning,       __FILE__, __func__, __LINE__, __VA_ARGS__);
#define LOG_ERROR(...)     gpi_log("cocotb.gpi", GPIError,         __FILE__, __func__, __LINE__, __VA_ARGS__);
#define LOG_CRITICAL(...)  gpi_log("cocotb.gpi", GPICritical,      __FILE__, __func__, __LINE__, __VA_ARGS__); 

/**
 * A GPI logging handler function.
 * See #gpi_log for other params.
 *
 * @param[in] userdata  State data used by the logging handler
 * @returns boolean representing success (0) or failure (non-0). If there is a failure, `gpi_log` will fall back on the native logger.
 */
typedef int (gpi_log_handler_type)(
    void *userdata,
    const char *name,
    int level,
    const char *pathname,
    const char *funcname,
    long lineno,
    const char *msg,
    va_list args);

/**
 * Log a message.
 * If a user log handler object is set, uses the registered log handler, otherwise uses the native logger.
 * @param[in] name      Name of the logger
 * @param[in] level     Level at which to log a message at
 * @param[in] pathname  Name of the file where the call site is located
 * @param[in] funcname  Name of the function where the call site is located
 * @param[in] lineno    Line number of the call site
 * @param[in] msg       The message to log, uses C-style format specifier
 * @param[in] ...       Additional arguments; formatted at inserted in message according to format specifier in msg argument
 */
void gpi_log(const char *name, int level, const char *pathname, const char *funcname, long lineno, const char *msg, ...);

/**
 * Retrieve current custom log handler.
 * @param[out] handler  Custom log handler registered previously, or NULL if no handler was set
 * @param[out] userdata Custom log handler userdata registered previously, or NULL if no handler was set
 */
void gpi_get_log_handler(gpi_log_handler_type **handler, void **userdata);

/**
 * Set custom log handler.
 * @param[in] handler   Handler function to call when the GPI logs a message
 * @param[in] userdata  State data to pass to the handler function when logging a message
 */
void gpi_set_log_handler(gpi_log_handler_type *handler, void *userdata);

/**
 * Clear current custom log handler and use native logger
 */
void gpi_clear_log_handler(void);

/**
 * Builtin logger implementation, used as a fallback when no custom log handler is set.
 * See #gpi_log for details on parameters.
 */
void gpi_native_logger_log(
    const char *name,
    int level,
    const char *pathname,
    const char *funcname,
    long lineno,
    const char *msg,
    ...);

/**
 * Set minimum logging level of the builtin logger implementation.
 * If a logging request occurs where the logging level is lower than the level set by this function, it is not logged.
 * *Only* affects the native logger.
 * @param[in] level     Logging level
 * @return              Previous logging level
 */
int gpi_native_logger_set_level(int level);

gpilog will contain a default implementation, the "native" logger, which will just use stdio. This native logger will be able to log messages and have it's log level set. Users may register their own loggers using gpi_set_log_handler which takes a handler function and an opaque userdata pointer. This should be sufficient for gpi_embed to register a Python logger implementation after it is loaded and remove the Python dependency in gpilog. The only thing the user may do with the custom log handler is clear it, to continue using the native logger, or retrieve it. The custom logger will not respect the native logger's log level.

I had thought about merging gpilog into GPI, but this is not possible because cocotb_utils depends upon gpilog. If gpi and gpilog were merged, cocotb_utils would then depend upon gpi. However, gpi depends upon cocotb_utils, so we encounter a loop. Everything could be merged into gpi, but I feel leaving it parsed out for now is fine.

[eric-wieser]

@param[in] ...       Additional arguments; interpretation depends on the handler

This strikes me as an unusable interface if specified this way. The caller of LOG_DEBUG(...) has no idea what the attached handle is, meaning they cannot safely use this argument.

[eric-wieser]

typedef void (gpi_log_handler_type)(

Should this be typedef int, to allow handlers to fail and fallback to the default? Or are they responsible for just calling the default handler in this case?

[ktbarrett]

@eric-wieser

This strikes me as an unusable interface if specified this way.

This is probably just bad wording, I copy-pasted it from a 5 month old branch that otherwise hasn't sen the light of day, it would work exactly like it already does.

EDIT: updated to reference C-style format strings.

Or are they responsible for just calling the default handler in this case?

That is how it is handled now.

[marlonjames]

I think it would be better to let the handler fail with a return and have gpi_log call the default native logger, so that the registered handler doesn't have to deal with va_copy.

[ktbarrett]

Well that's 2 agreements. Updated.

[marlonjames]

Shouldn't gpi_native_logger_log take a va_list? The idea is to always go through gpi_log, right?

[ktbarrett]

The idea is that a user could go straight to the native logger instead of using gpi_log. If that's not necessary, I'd just remove it. There would be a private version that takes a va_list that would be called by gpi_log. If the user isn't manually calling the native logger as a fall back (gpi_log handles that automatically as recommend above) there isn't a need to expose a va_list version to the user.

[eric-wieser]

Well that's 2 agreements. Updated.

Mine was more of an exploratory question. The current approach is perhaps better because it allows:

try:
   do_my_logging(*args)
except:
   do_the_default_logging("inside knowledge about why logging failed")
   do_the_default_logging(*args)

If you switch to a return code, then you can no longer support this approach.

[ktbarrett]

2000 closed this by way of #2268