Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14670017:
Start a whitelist of intrinsics for the PNaCl ABI checker. (Closed)
|
Created:
7 years, 7 months ago by jvoung (off chromium) Modified:
7 years, 7 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionStart a whitelist of intrinsics for the PNaCl ABI checker.
This list is currently too small to support our tests
(scons, gcc, llvm). To prevent the tests from breaking,
there is a -pnaclabi-allow-intrinsics flag which defaults
to true. To turn on the checking for real,
set -pnaclabi-allow-intrinsics=0.
We will avoid actually using the -pnaclabi-allow-intrinsics
flags in tests, and try to just stick with the
-pnacl-disable-abi-check flag. Remove this flag soon.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3378
R=eliben@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=77cc10f
Patch Set 1 #
Total comments: 9
Patch Set 2 : organize more #
Total comments: 6
Patch Set 3 : be more explicit about what tests are known to use #Patch Set 4 : comment #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:36: static cl::opt<bool> PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics", Can we divide intrinsics into 3 groups: 1. Those that we allow (and will allow in the stable ABI) 2. Those we don't allow even now (i.e. won't fail any existing tests) 3. Those that we plan to disallow bu have to adjust tests first Which of these is the flag meant to override? 2, or or both? In any case the current flag name is not very descriptive as it's not clear that some intrinsics are allowed even if it's not set to 1. https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:197: if (MI->isIntrinsic()) { Can an intrinsic be called without a declaration? What if we don't run the IR verifier in the translator? https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics.ll File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics... test/NaCl/PNaClABI/intrinsics.ll:1: ; RUN: pnacl-abicheck -pnaclabi-allow-intrinsics=0 < %s | FileCheck %s I guess at least one sanity check with this flag=1 could be useful to catch problems quickly
https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:36: static cl::opt<bool> PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics", On 2013/05/07 15:27:40, eliben wrote: > Can we divide intrinsics into 3 groups: > > 1. Those that we allow (and will allow in the stable ABI) > 2. Those we don't allow even now (i.e. won't fail any existing tests) > 3. Those that we plan to disallow bu have to adjust tests first > > Which of these is the flag meant to override? 2, or or both? In any case the > current flag name is not very descriptive as it's not clear that some intrinsics > are allowed even if it's not set to 1. That sounds like a good breakdown. I'll rename the flag to "PNaClABIAllowDevIntrinsics"? Other suggestions? The flag is intended for (3), but patch set 1 didn't enumerate stuff for (2). Just added a couple of things for (2). For (3) we could also consider some intrinsics for debugging, or it might be intrinsics that we can support, but need more thought or testing (e.g., __builtin_popcount) which is why I considered calling the flag "Dev". For (2) I partly ended up having to glob based on function name, instead of using the switch-case facilities. There are probably over a thousand intrinsics in llvm (llvm.arm.*, llvm.x86.*, llvm.ppc.*, llvm.nvvm.* ). The implementation of the table-gen'ed llvm::Function::getIntrinsicID() takes up 170KB of .text for the x86-64 build of the LLC.nexe. https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:197: if (MI->isIntrinsic()) { On 2013/05/07 15:27:40, eliben wrote: > Can an intrinsic be called without a declaration? I don't think you can call intrinsics without a declaration: "error: use of undefined value '@llvm.readcyclecounter'" > What if we don't run the IR verifier in the translator? Can you elaborate? Does the usage of intrinsics differ between code that the translator sees and code that "opt" or "pnacl-abicheck" sees? https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics.ll File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics... test/NaCl/PNaClABI/intrinsics.ll:1: ; RUN: pnacl-abicheck -pnaclabi-allow-intrinsics=0 < %s | FileCheck %s On 2013/05/07 15:27:40, eliben wrote: > I guess at least one sanity check with this flag=1 could be useful to catch > problems quickly Done.
On 2013/05/07 17:39:46, jvoung (cr) wrote: > https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... > File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): > > https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:36: static cl::opt<bool> > PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics", > On 2013/05/07 15:27:40, eliben wrote: > > Can we divide intrinsics into 3 groups: > > > > 1. Those that we allow (and will allow in the stable ABI) > > 2. Those we don't allow even now (i.e. won't fail any existing tests) > > 3. Those that we plan to disallow bu have to adjust tests first > > > > Which of these is the flag meant to override? 2, or or both? In any case the > > current flag name is not very descriptive as it's not clear that some > intrinsics > > are allowed even if it's not set to 1. > > That sounds like a good breakdown. > > > I'll rename the flag to "PNaClABIAllowDevIntrinsics"? Other suggestions? The > flag is intended for (3), but patch set 1 didn't enumerate stuff for (2). Just > added a couple of things for (2). > Dev is better. Another suggestion - Unstable? Whatever feels better to you, I don't mind. > > For (3) we could also consider some intrinsics for debugging, or it might be > intrinsics that we can support, but need more thought or testing (e.g., > __builtin_popcount) which is why I considered calling the flag "Dev". > > > For (2) I partly ended up having to glob based on function name, instead of > using the switch-case facilities. There are probably over a thousand intrinsics > in llvm (llvm.arm.*, llvm.x86.*, llvm.ppc.*, llvm.nvvm.* ). The implementation > of the table-gen'ed llvm::Function::getIntrinsicID() takes up 170KB of .text for > the x86-64 build of the LLC.nexe. Isn't the point of whitelisting to avoid this globbing? I was under the impression we're banning the vast majority of intrinsics? > > https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:197: if (MI->isIntrinsic()) { > On 2013/05/07 15:27:40, eliben wrote: > > Can an intrinsic be called without a declaration? > > I don't think you can call intrinsics without a declaration: > > "error: use of undefined value '@llvm.readcyclecounter'" > > > > What if we don't run the IR verifier in the translator? > > Can you elaborate? Does the usage of intrinsics differ between code that the > translator sees and code that "opt" or "pnacl-abicheck" sees? > What I mean is this crooked case: we get a pexe where an intrinsic is used but not declared (and the verifier doesn't run so it doesn't check this). Could we then not catch an invalid intrinsic and have the codegen happily accepting it? It's just a thought as I'm unsure about the exact mechanics. Very likely to be a non-issue. > https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics.ll > File test/NaCl/PNaClABI/intrinsics.ll (right): > > https://codereview.chromium.org/14670017/diff/1/test/NaCl/PNaClABI/intrinsics... > test/NaCl/PNaClABI/intrinsics.ll:1: ; RUN: pnacl-abicheck > -pnaclabi-allow-intrinsics=0 < %s | FileCheck %s > On 2013/05/07 15:27:40, eliben wrote: > > I guess at least one sanity check with this flag=1 could be useful to catch > > problems quickly > > Done.
https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:38: cl::desc("Allow all LLVM intrinsics duing PNaCl ABI verification."), duing https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:148: DEBUG(dbgs() << "Unknown intrinsic: " << F->getName() << "\n"); Again, I'm not sure why we can't: Detect (1) and (3) in the switch, leaving the rest to 'default'. Are there very many intrinsics in (3) ? https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:227: if (MI->isIntrinsic()) { Any reason not to combine into a single condition?
https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:36: static cl::opt<bool> PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics", On 2013/05/07 17:39:46, jvoung (cr) wrote: > On 2013/05/07 15:27:40, eliben wrote: > > Can we divide intrinsics into 3 groups: > > > > 1. Those that we allow (and will allow in the stable ABI) > > 2. Those we don't allow even now (i.e. won't fail any existing tests) > > 3. Those that we plan to disallow bu have to adjust tests first > > > > Which of these is the flag meant to override? 2, or or both? In any case the > > current flag name is not very descriptive as it's not clear that some > intrinsics > > are allowed even if it's not set to 1. > > That sounds like a good breakdown. > > > I'll rename the flag to "PNaClABIAllowDevIntrinsics"? Other suggestions? The > flag is intended for (3), but patch set 1 didn't enumerate stuff for (2). Just > added a couple of things for (2). > > > For (3) we could also consider some intrinsics for debugging, or it might be > intrinsics that we can support, but need more thought or testing (e.g., > __builtin_popcount) which is why I considered calling the flag "Dev". > > > For (2) I partly ended up having to glob based on function name, instead of > using the switch-case facilities. There are probably over a thousand intrinsics > in llvm (llvm.arm.*, llvm.x86.*, llvm.ppc.*, llvm.nvvm.* ). The implementation > of the table-gen'ed llvm::Function::getIntrinsicID() takes up 170KB of .text for > the x86-64 build of the LLC.nexe. At the Tues meeting (an hour ago), there was some desire to not have extra flags for the verifier. This can be done two ways: (a) disable tests (b) just use the existing "--pnacl-disable-abi-check" (c) gain enough confidence to start accepting stuff in list (3) -- possibly we could duplicate the test coverage via some llvm lit tests that show, for each architecture, what architectural support we have, or what compiler_rt support we have. ===== How about I keep this flag here for now, and remove it once there is enough of a combination of (a), (b), and (c). This flag won't actually propagate out into scons, gcc, or llvm test suites, only (a) or (b) will be used to deal with them. I'd like to start testing and promoting some of the stuff in (3) so that we won't need to do too much of (a) and (b). We could also get rid of this flag now, and just "return true;" for stuff in category (3) for now. https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:38: cl::desc("Allow all LLVM intrinsics duing PNaCl ABI verification."), On 2013/05/07 17:49:36, eliben wrote: > duing Done. https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:148: DEBUG(dbgs() << "Unknown intrinsic: " << F->getName() << "\n"); On 2013/05/07 17:49:36, eliben wrote: > Again, I'm not sure why we can't: > > Detect (1) and (3) in the switch, leaving the rest to 'default'. Are there very > many intrinsics in (3) ? Added the list of stuff triggered by our scons + gcc + llvm nightly + spec tests. https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:227: if (MI->isIntrinsic()) { On 2013/05/07 17:49:36, eliben wrote: > Any reason not to combine into a single condition? Done.
On 2013/05/07 21:24:41, jvoung (cr) wrote: > https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... > File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): > > https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:36: static cl::opt<bool> > PNaClABIAllowIntrinsics("pnaclabi-allow-intrinsics", > On 2013/05/07 17:39:46, jvoung (cr) wrote: > > On 2013/05/07 15:27:40, eliben wrote: > > > Can we divide intrinsics into 3 groups: > > > > > > 1. Those that we allow (and will allow in the stable ABI) > > > 2. Those we don't allow even now (i.e. won't fail any existing tests) > > > 3. Those that we plan to disallow bu have to adjust tests first > > > > > > Which of these is the flag meant to override? 2, or or both? In any case the > > > current flag name is not very descriptive as it's not clear that some > > intrinsics > > > are allowed even if it's not set to 1. > > > > That sounds like a good breakdown. > > > > > > I'll rename the flag to "PNaClABIAllowDevIntrinsics"? Other suggestions? The > > flag is intended for (3), but patch set 1 didn't enumerate stuff for (2). > Just > > added a couple of things for (2). > > > > > > For (3) we could also consider some intrinsics for debugging, or it might be > > intrinsics that we can support, but need more thought or testing (e.g., > > __builtin_popcount) which is why I considered calling the flag "Dev". > > > > > > For (2) I partly ended up having to glob based on function name, instead of > > using the switch-case facilities. There are probably over a thousand > intrinsics > > in llvm (llvm.arm.*, llvm.x86.*, llvm.ppc.*, llvm.nvvm.* ). The > implementation > > of the table-gen'ed llvm::Function::getIntrinsicID() takes up 170KB of .text > for > > the x86-64 build of the LLC.nexe. > > > At the Tues meeting (an hour ago), there was some desire to not have extra flags > for the verifier. > > This can be done two ways: > > (a) disable tests > > (b) just use the existing "--pnacl-disable-abi-check" > > (c) gain enough confidence to start accepting stuff in list (3) -- possibly we > could duplicate the test coverage via some llvm lit tests that show, for each > architecture, what architectural support we have, or what compiler_rt support we > have. > > > ===== > > How about I keep this flag here for now, and remove it once there is enough of a > combination of (a), (b), and (c). > Sounds fine. Mention this in the issue so it won't be closed before the right flags go into the right places. > This flag won't actually propagate out into scons, gcc, or llvm test suites, > only (a) or (b) will be used to deal with them. > > I'd like to start testing and promoting some of the stuff in (3) so that we > won't need to do too much of (a) and (b). We could also get rid of this flag > now, and just "return true;" for stuff in category (3) for now. > > https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... > File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): > > https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:38: cl::desc("Allow all LLVM > intrinsics duing PNaCl ABI verification."), > On 2013/05/07 17:49:36, eliben wrote: > > duing > > Done. > > https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:148: DEBUG(dbgs() << "Unknown > intrinsic: " << F->getName() << "\n"); > On 2013/05/07 17:49:36, eliben wrote: > > Again, I'm not sure why we can't: > > > > Detect (1) and (3) in the switch, leaving the rest to 'default'. Are there > very > > many intrinsics in (3) ? > > Added the list of stuff triggered by our scons + gcc + llvm nightly + spec > tests. > > https://codereview.chromium.org/14670017/diff/6001/lib/Analysis/NaCl/PNaClABI... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:227: if (MI->isIntrinsic()) { > On 2013/05/07 17:49:36, eliben wrote: > > Any reason not to combine into a single condition? > > Done.
lgtm
https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:197: if (MI->isIntrinsic()) { On 2013/05/07 17:39:46, jvoung (cr) wrote: > On 2013/05/07 15:27:40, eliben wrote: > > Can an intrinsic be called without a declaration? > > I don't think you can call intrinsics without a declaration: > > "error: use of undefined value '@llvm.readcyclecounter'" > > > > What if we don't run the IR verifier in the translator? > > Can you elaborate? Does the usage of intrinsics differ between code that the > translator sees and code that "opt" or "pnacl-abicheck" sees? Ah yes, you mean we'd do something like "llc -disable-verify". The error I showed above is actually from the LLParser. We don't support textual llvm IR, so we won't see that particular error. I don't think you can generate a binary representation of the bitcode that calls a function that hasn't been declared. According to "llvm-bcanalyzer -dump ..." you get: <FUNCTION_BLOCK ...> <DECLAREBLOCKS ...> <INST_CALL op0=... op2=[[FUNCTION ID]] /> ... </FUNCTION_BLOCK> where [[FUNCTION ID]] should be something defined earlier.
https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/14670017/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:197: if (MI->isIntrinsic()) { On 2013/05/07 22:32:18, jvoung (cr) wrote: > On 2013/05/07 17:39:46, jvoung (cr) wrote: > > On 2013/05/07 15:27:40, eliben wrote: > > > Can an intrinsic be called without a declaration? > > > > I don't think you can call intrinsics without a declaration: > > > > "error: use of undefined value '@llvm.readcyclecounter'" > > > > > > > What if we don't run the IR verifier in the translator? > > > > Can you elaborate? Does the usage of intrinsics differ between code that the > > translator sees and code that "opt" or "pnacl-abicheck" sees? > > Ah yes, you mean we'd do something like "llc -disable-verify". The error I > showed above is actually from the LLParser. We don't support textual llvm IR, > so we won't see that particular error. > > I don't think you can generate a binary representation of the bitcode that calls > a function that hasn't been declared. > > According to "llvm-bcanalyzer -dump ..." you get: > > <FUNCTION_BLOCK ...> > <DECLAREBLOCKS ...> > <INST_CALL op0=... op2=[[FUNCTION ID]] /> > ... > </FUNCTION_BLOCK> > > where [[FUNCTION ID]] should be something defined earlier. Err... not that you can't generate bad binary bitcode, but maybe the bitcode reader would blow up if you did. I'm not sure =)
Message was sent while issue was closed.
Committed patchset #4 manually as r77cc10f (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
