[BitcodeReader] datalayout must be specified before it is queried.

[efriedma]

Summary

This isn't really a new invariant; it effectively already existed due to existing DataLayout queries. But this makes it explicit.

This is technically not backward-compatible with the existing bitcode reader, but it's backward-compatible with the output of the bitcode writer, which is what matters in practice.

No testcase because I don't know a good way to write one.

Split off from D78403. (And modified a bit to handle the interaction between datalayout parsing and DataLayout::getProgramAddressSpace().)

[arsenm]

We do have a bunch of tests that partially specify the datalayout to work around not properly setting it from the command line or target machine.

If you remove the datalayout string from test/CodeGen/AMDGPU/vector-alloca-bitcast.ll for example, does it work with this patch?

[efriedma]

If you remove the datalayout string from test/CodeGen/AMDGPU/vector-alloca-bitcast.ll for example, does it work with this patch?

This patch doesn't have any effect except to reject certain unusual bitcode files not generated by LLVM.

If you're asking about D78403, that doesn't affect the general rules for what tools overwrite the datalayout based on the TargetMachine. Currently, llc always does this, and opt never does this. So the error messages for that sort of thing should remain the same.

[jdoerfert]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

Should we emit an error here? I mean, can it happen we now silently ignore the DL or triple and the user is baffled what is happening?

[efriedma]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

We print an error if we see MODULE_CODE_DATALAYOUT or MODULE_CODE_TRIPLE after ResolveDataLayout runs.

[jdoerfert]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

I guess we could test that?

[mehdi_amini]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

That would require crafting manually some invalid bitcode wouldn't it?

[mehdi_amini]

LGTM (but worth seeing if @pcc has an opinion on this)

[jdoerfert]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

Yes, or better, something we now call invalid. @efriedma please correct me but there could be an input we accepted before but not now?

[efriedma]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

Yes. But no tools exist to generate such a file.

I don't really want to sink a day into constructing a framework for BitcodeReader unittests, but if you think it's necessary, I guess I can.

[jdoerfert]

LGTM

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3445

No, it was more of an idea, not a request.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Error BitcodeReader::parseModule(uint64_t ResumeBit,
   if (ResumeBit) {
     if (Error JumpFailed = Stream.JumpToBit(ResumeBit))
       return JumpFailed;
   } else if (Error Err = Stream.EnterSubBlock(bitc::MODULE_BLOCK_ID))
     return Err;
 
   SmallVector<uint64_t, 64> Record;
 
+  // Parts of bitcode parsing depend on the datalayout.  Make sure we
+  // finalize the datalayout before we run any of that code.
+  bool ResolvedDataLayout = false;
+  auto ResolveDataLayout = [&] {
+    if (ResolvedDataLayout)
+      return;
+
+    // datalayout and triple can't be parsed after this point.
+    ResolvedDataLayout = true;
+
+    // Upgrade data layout string.
+    std::string DL = llvm::UpgradeDataLayoutString(
+        TheModule->getDataLayoutStr(), TheModule->getTargetTriple());
+    TheModule->setDataLayout(DL);
+  };
+
   // Read all the records for this module.
   while (true) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
     if (!MaybeEntry)
       return MaybeEntry.takeError();
     llvm::BitstreamEntry Entry = MaybeEntry.get();
 
     switch (Entry.Kind) {
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
+      ResolveDataLayout();
       return globalCleanup();
 
     case BitstreamEntry::SubBlock:
       switch (Entry.ID) {
       default:  // Skip unknown content.
         if (Error Err = Stream.SkipBlock())
           return Err;
         break;
case BitstreamEntry::SubBlock:
         if (Error Err = MDLoader->parseModuleMetadata())
           return Err;
         break;
       case bitc::METADATA_KIND_BLOCK_ID:
         if (Error Err = MDLoader->parseMetadataKinds())
           return Err;
         break;
       case bitc::FUNCTION_BLOCK_ID:
+        ResolveDataLayout();
+
         // If this is the first function body we've seen, reverse the
         // FunctionsWithBodies list.
         if (!SeenFirstFunctionBody) {
           std::reverse(FunctionsWithBodies.begin(), FunctionsWithBodies.end());
           if (Error Err = globalCleanup())
             return Err;
           SeenFirstFunctionBody = true;
         }
while (true) {
     case bitc::MODULE_CODE_VERSION: {
       Expected<unsigned> VersionOrErr = parseVersionRecord(Record);
       if (!VersionOrErr)
         return VersionOrErr.takeError();
       UseRelativeIDs = *VersionOrErr >= 1;
       break;
     }
     case bitc::MODULE_CODE_TRIPLE: {  // TRIPLE: [strchr x N]
+      if (ResolvedDataLayout)
+        return error("target triple too late in module");
       std::string S;
       if (convertToString(Record, 0, S))
         return error("Invalid record");
       TheModule->setTargetTriple(S);
       break;
     }
     case bitc::MODULE_CODE_DATALAYOUT: {  // DATALAYOUT: [strchr x N]
+      if (ResolvedDataLayout)
+        return error("datalayout too late in module");
       std::string S;
       if (convertToString(Record, 0, S))
         return error("Invalid record");
       TheModule->setDataLayout(S);
       break;
     }
     case bitc::MODULE_CODE_ASM: {  // ASM: [strchr x N]
       std::string S;
case bitc::MODULE_CODE_COMDAT:
       if (Error Err = parseComdatRecord(Record))
         return Err;
       break;
     case bitc::MODULE_CODE_GLOBALVAR:
       if (Error Err = parseGlobalVarRecord(Record))
         return Err;
       break;
     case bitc::MODULE_CODE_FUNCTION:
+      ResolveDataLayout();
       if (Error Err = parseFunctionRecord(Record))
         return Err;
       break;
     case bitc::MODULE_CODE_IFUNC:
     case bitc::MODULE_CODE_ALIAS:
     case bitc::MODULE_CODE_ALIAS_OLD:
       if (Error Err = parseGlobalIndirectSymbolRecord(BitCode, Record))
         return Err;
while (true) {
     case bitc::MODULE_CODE_SOURCE_FILENAME:
       SmallString<128> ValueName;
       if (convertToString(Record, 0, ValueName))
         return error("Invalid record");
       TheModule->setSourceFileName(ValueName);
       break;
     }
     Record.clear();
-
-    // Upgrade data layout string.
-    std::string DL = llvm::UpgradeDataLayoutString(
-        TheModule->getDataLayoutStr(), TheModule->getTargetTriple());
-    TheModule->setDataLayout(DL);
   }
 }
 
 Error BitcodeReader::parseBitcodeInto(Module *M, bool ShouldLazyLoadMetadata,
                                       bool IsImporting) {
   TheModule = M;
   MDLoader = MetadataLoader(Stream, *M, ValueList, IsImporting,
                             [&](unsigned ID) { return getTypeByID(ID); });