[PATCH] D78403: Infer alignment of loads with unspecified alignment in IR/bitcode parsing.

[Eli Friedman via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:108
+        LLVMContext &Context, DataLayoutCallbackTy DataLayoutCallback =
+                                  [](StringRef) { return None; });
 
----------------
mehdi_amini wrote:
> I think a better default is an empty function_ref instead of a lambda: I suspect you can write it this way `DataLayoutCallbackTy DataLayoutCallback = {}` 
I didn't realize function_ref was allowed to be null.

I guess that's simpler in some sense, but I'm not sure it's worthwhile; possibly-null values make the documentation more complicated.  Not a big deal either way, though.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3626
+      if (SeenFirstFunctionBody)
+        return error("datalayout too late in module");
       std::string S;
----------------
mehdi_amini wrote:
> Are you making invalid bitcode that was valid before here?
> I'd rather avoid doing this, can we check `if (SeenFirstFunctionBody && DataLayoutCallback)`?
> (which also implies to not provide a callback from any tool when not needed (and make sure the default argument for the callback is everywhere an empty function_ref)
Yes, this is rejecting bitcode that LLVM currently accepts.  No version of LLVM has ever emitted bitcode that violates this constraint, though; LLVM always emits the datalayout early.  In theory, I guess if someone had a custom bitcode emitter that emitted bitcode in some really weird order, it could be an issue.

I'm not sure what we would do if we want to "handle" this; if we see a datalayout after we've already read IR instructions, we would have to go back and fix/reparse already parsed IR.  I guess we could, but I'm not really enthusiastic about writing bitcode testcases by hand to test that codepath.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4846
+      if (!Align && !Ty->isSized())
+        return error("load of unsized type");
+      if (!Align)
----------------
mehdi_amini wrote:
> Is there valid bitcode out-there that could be hit by this?
No, it's illegal to load a value of unsized type.  The verifier has an equivalent check; we're just moving that check earlier, so we can resolve the alignment before calling the LoadInst constructor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78403/new/

https://reviews.llvm.org/D78403