[PATCH] D52023: [ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm-c/lto.h:828
 extern void thinlto_codegen_set_cache_size_bytes(thinlto_code_gen_t cg,
                                                  unsigned max_size_bytes);
 
----------------
tejohnson wrote:
> jhenderson wrote:
> > tejohnson wrote:
> > > Shouldn't this be changed to uint64_t? I don't think that would break any existing users since it is wider.
> > That's what I originally thought (and I would prefer to do it if possible) but I think there are two issues, which may both be irrelevant:
> > 1) Is uint64_t definitely available in all versions of C that this header is intended to be compatible with? I don't know what the compatibility guarantees are.
> > 2) In the past, my team have occasionally updated the LTO library (as a DLL) without updating the header in our linker's source tree. If somebody were to do this in the future, (from one with the old, smaller size, to one with the newer size), I think this would result in unexpected behaviour (because the caller thinks it's calling an unsigned version, but the function called actually takes a uint64_t). In our case, that won't happen, since I'll be updating our local header once we integrate these changes in our downstream port, but I wasn't sure if this is something that might affect other users. I couldn't find any documentation explaining the compatibility guarantees of the C interface.
> Those are good questions and I don't know the answers as I don't have a lot of familiarity with the C interface. Hopefully someone else knows the compatibility guarantees there.
IIRC: it is expected to preserve ABI, or when impossible, bump the version number (the linker is expected then to check it at runtime and adjust its behavior)


Repository:
  rL LLVM

https://reviews.llvm.org/D52023