[PATCH] D27507: [ThinLTO] Add an API to trigger file-based API for returning objects to the linker

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini marked 3 inline comments as done.
mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:87
    */
   std::vector<std::unique_ptr<MemoryBuffer>> &getProducedBinaries() {
     return ProducedBinaries;
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > tejohnson wrote:
> > > Give an error here if the wrong routine called? I.e. if getProducedBinaries called and setGeneratedObjectsDirectory was called. I see that the C api will assert if the wrong interface is called because the index will be out of range, but perhaps a more meaningful error could be given.
> > There shouldn't be an assert in the C API if correctly used: 
> > 
> > The client is supposed to iterate over the range given by `thinlto_module_get_num_object_files()` and call `thinlto_module_get_object_file()` (similarly `thinlto_module_get_object`/`thinlto_module_get_num_objects`).
> > 
> > The way I implemented it in ld64 is basically:
> > 
> > ```
> > for (int BufId = 0; BufID < thinlto_module_get_num_objects; ++BufID) {
> >    process(thinlto_module_get_object(BufID));
> > }
> > for (int FileID = 0; FileID < thinlto_module_get_num_object_files; ++FileID) {
> >    process(thinlto_module_get_object_file(FileID));
> > }
> > ```
> > 
> > Does it make sense?
> Sure, it won't assert if invoked properly, I was suggesting a more meaningful error in case it is not.
I just don't understand what error you're suggesting here, can you clarify?

Right now this API is always OK to be called, and  both the `thinlto_module_get_num_object_files` and `thinlto_module_get_num_objects` are invoking it during the normal use I mentioned above (loop bound check).
That's why the C API asserts only when you access out-of-bound and not here when you access the vector itself (to check for size for instance).


https://reviews.llvm.org/D27507