[PATCH] LTO: introduce object file-based on-disk module format.

[Peter Collingbourne]

On Fri, Jul 04, 2014 at 04:44:42AM +0300, Alp Toker wrote:
>>   +static StringRef getObjectBitcodeData(ObjectFile *obj,
>> +                                      std::string *errMsg = 0) {
>> +  for (auto sec : obj->sections()) {
>> +    StringRef secName;
>> +    if (std::error_code ec = sec.getName(secName)) {
>> +      if (errMsg)
>> +        *errMsg = ec.message();
>> +      return StringRef();
>> +    }
>> +    if (secName == ".llvmbc") {
>> +      StringRef secContents;
>> +      if (std::error_code ec = sec.getContents(secContents)) {
>> +        if (errMsg)
>> +          *errMsg = ec.message();
>> +        return StringRef();
>> +      }
>> +      return secContents;
>> +    }
>> +  }
>> +
>> +  if (errMsg)
>> +    *errMsg = "section .llvmbc not found";
>> +  return StringRef();
>> +}
>
> I feel the object file-based bitcode support should be layered in to a  
> separate source file.

Right, I'll probably end up moving some of this code into lib/Object per
Rafael's suggestion.

>
>> +
>> +static StringRef getBitcodeData(StringRef str, std::string *errMsg = 0) {
>> +  switch (sys::fs::identify_magic(str)) {
>> +  case sys::fs::file_magic::bitcode:
>> +    return str;
>> +  case sys::fs::file_magic::elf_relocatable:
>> +  case sys::fs::file_magic::macho_object:
>> +  case sys::fs::file_magic::coff_object: {
>> +    std::unique_ptr<MemoryBuffer> buffer(
>> +        MemoryBuffer::getMemBuffer(str, "", false));
>> +    auto obj = ObjectFile::createObjectFile(buffer);
>> +    if (!obj) {
>> +      if (errMsg)
>> +        *errMsg = obj.getError().message();
>> +      return StringRef();
>> +    }
>> +    return getObjectBitcodeData(*obj, errMsg);
>> +  }
>> +  default:
>> +    return StringRef();
>> +  }
>> +}
>> +
>>   /// isBitcodeFile - Returns 'true' if the file (or memory contents) is LLVM
>>   /// bitcode.
>>   bool LTOModule::isBitcodeFile(const void *mem, size_t length) {
>> -  return sys::fs::identify_magic(StringRef((const char *)mem, length)) ==
>> -         sys::fs::file_magic::bitcode;
>> +  return !getBitcodeData(StringRef((const char *)mem, length)).empty();
>
> This is potentially less efficient. Why change it from using magic?

It *is* using magic to check the file type, see the code above. The new
code needs to check for the presence of the .llvmbc section if it sees an
object file.

>
>>   }
>>     bool LTOModule::isBitcodeFile(const char *path) {
>>     sys::fs::file_magic type;
>>     if (sys::fs::identify_magic(path, type))
>>       return false;
>> -  return type == sys::fs::file_magic::bitcode;
>> +  switch (type) {
>> +  case sys::fs::file_magic::bitcode:
>> +    return true;
>> +  case sys::fs::file_magic::elf_relocatable:
>> +  case sys::fs::file_magic::macho_object:
>> +  case sys::fs::file_magic::coff_object: {
>> +    auto Obj = ObjectFile::createObjectFile(path);
>> +    if (!Obj)
>> +      return false;
>> +    return !getObjectBitcodeData(*Obj).empty();
>
> I don't like this at all. The behaviour of existing isBitcodeFile()  
> shouldn't just change.
>
> This looks much more like it should be a isObjectFileWithBitcode() that  
> consumers can optionally call. It's really up to the library user what  
> input formats they want to accept.

There is already a way for a client to check whether a file is a pure bitcode
file, namely by reading the magic. Moreover, we have no LTO clients in tree
which need to support one format and not the other.

Thanks,
-- 
Peter