microsoft / igvm

MIT License
81 stars 17 forks source link

igvm_c: Ensure C header types are prefixed with IGVM #52

Closed roy-hopkins closed 2 months ago

roy-hopkins commented 2 months ago

The generated header file igvm_defs.h contains a number of definitions that could potentially clash with other non-IGVM header files, for example "u32_le". This change ensures that all types defined in the generated header file are prefixed with IGVM/Igvm.

A new post-processing script needed to added to implement some of these changes as cbindgen does not have the granularity to make the necessary changes. In addition, some annototations needed to be added to the rust code itself.

Note that this is a breaking change to the header file, but a necessary one that is being introduced before adoption of the IGVM C API.

berrange commented 2 months ago

The igvm_defs.h header looks good to me now. The igvm.h header has a couple of remaining constants to namespace:

/**
 * Number of bytes in a page for X64.
 */
#define X64_PAGE_SIZE 4096

/**
 * Number of bytes in a large page for X64.
 */
#define X64_LARGE_PAGE_SIZE 2097152

/**
 * Number of bytes in a 1GB page for X64.
 */
#define X64_1GB_PAGE_SIZE 1073741824

Unrelated to this PR, but something to fix

#ifndef IGVM_DEFS_H
#define IGVM_DEFS_H

#pragma once

The #ifndef/#define dance is redundant if you're using #pragma once, or vica-versa. Pick one approach only for igvm_defs.h. The igvm.h file lacks the #pragma once usage meanwhile - consistency would be good.

Also the #include <std*.h> in igvm.h are redundant, given it is including igvm_defs.h which already pulls in these system headers. If this is something cbindgen does unconditionally which can't be turned off though, its no big deal and not worth fixing.

roy-hopkins commented 2 months ago

The igvm.h header has a couple of remaining constants to namespace:

Thanks - I missed those. I've added a new commit to fix the definitions in igvm.h. In addition to the ones you highlighted, I've also renamed HEADER_SECTION_* to IGVM_HEADER_SECTION_*.

The #ifndef/#define dance is redundant

I've updated the original commit to remove the #pragma to make it consistent.

Also the #include <std*.h> in igvm.h are redundant

I've added another commit which removes these from both generated header files. I think it makes more sense for the consumer of the IGVM library to include the required header files.

berrange commented 2 months ago

Also the #include <std*.h> in igvm.h are redundant I've added another commit which removes these from both generated header files. I think it makes more sense for the consumer of the IGVM library to include the required header files.

Hmm, not sure I'd agree with that. IMHO best practice for C header files is for them to be self contained. Consider if an application with the current codebase release has to only add #include <stdint.h> to their .c file today before including igvm.h. Then in the next release igvm_defs.h gains something that also requires #include <blah.h> as a pre-requisite. The existing application now has a broken build if used against the new igvm release.

Keeping the <std*.h> in igvm_defs.h keeps everything self contained and prevent future breakage possibility. The includes were only redundant in igvm.h.

roy-hopkins commented 2 months ago

Hmm, not sure I'd agree with that. IMHO best practice for C header files is for them to be self contained.

Ok. Reverted that. The include files are now only present in igvm_defs.h.

berrange commented 2 months ago

Looks good to me now.

chris-oo commented 2 months ago

Changes LGTM, I'll update your branch for you and merge.