[PATCH] D78403: Infer alignment of loads with unspecified alignment in IR/bitcode parsing.
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 22 13:37:30 PDT 2020
efriedma marked an inline comment as done.
efriedma added inline comments.
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3626
+ if (SeenFirstFunctionBody)
+ return error("datalayout too late in module");
std::string S;
----------------
mehdi_amini wrote:
> efriedma wrote:
> > 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.
> > if we see a datalayout after we've already read IR instructions, we would have to go back and fix/reparse already parsed IR
>
> This would already be an issue with the current parser though?
> If you don't mind committing these two lines right now since it seems like a defensive measure in the reader that is valid to do independently of this patch (assuming I understand correctly here?)
The current parser doesn't really query the datalayout. (Well, except in the cases where it does, for the program/stack address spaces. But that doesn't affect mainstream targets.)
Yes, sure, I can commit this separately.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78403/new/
https://reviews.llvm.org/D78403
More information about the llvm-commits
mailing list