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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:87
    */
   std::vector<std::unique_ptr<MemoryBuffer>> &getProducedBinaries() {
     return ProducedBinaries;
----------------
mehdi_amini wrote:
> 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).
Oh I see, you need to access the maps to get the size in order to guard/index the calls appropriately. So what I had in mind won't work. I was just trying to see if there was a more meaningful error other than the ones in thinlto_module_get_object_file() and the like than "Index overflow" when the wrong map was accessed. Maybe not worth it...


https://reviews.llvm.org/D27507