Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
807643002:
Don't allow instructions/globals to use alignment > 2**29. (Closed)
|
Created:
6 years ago by Karl Modified:
6 years ago Reviewers:
jvoung (off chromium) CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionDon't allow instructions/globals to use alignment > 2**29.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3998
R=jvoung@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=0b3bf5655ca7e4c68107041cbc5d7d8642768df7
Patch Set 1 #Patch Set 2 : Fix nit and add test cases. #
Total comments: 16
Patch Set 3 : Fix issues in last patch set. #Patch Set 4 : Fix nits. #
Total comments: 11
Patch Set 5 : Fix issues in patch set 4. #
Total comments: 11
Patch Set 6 : Fix issues in patch set 5. #
Messages
Total messages: 14 (1 generated)
kschimpf@google.com changed reviewers: + jvoung@chromium.org
https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:188: // Note: Alignement = 2 ** (Exponent - 1). Alignement -> Alignment https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:189: if (Exponent == 0) { Hmm, I don't think we ever said that "alloca" could not have an align of 0. I think only for load/store was that ever checked by the ABI checker. I'm not sure what C/C++ source would give you an alloca with align 0, but it might better to continue to allow 0. Is the reason for this the "Exp - 1" vs the "(1 << x) >> 1" ? https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:193: if (Exponent > 30) { // Note: Exponent is one larger than actual. "than actual" -> "than the limit"? https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:193: if (Exponent > 30) { // Note: Exponent is one larger than actual. include/llvm/IR/Value.h has "MaximumAlignment", which is "29", so you may be able to use that as the constant. The downside of using that is if it changes out from under us. Maybe use that + 1, and static assert that it is 29? If upstream changes it, we could then fork the limit or follow... WDYT? https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1549: getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(), A little ambivalent about the extra checks -- I don't think it would be easy to switch the check on here, but switch it off later when the abi checker runs, so that there isn't new overhead. On the other hand, the overhead is *probably* negligible. https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:82: /// Tests if we recognize when alignment gets too large. nit: maybe change Alloc - >"Alloca" to be a bit more specific (vs like malloc allocation) https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:135: // Show what happens when changing alignment to 30. "to 30" -> "to 2**30" ? https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:138: // Note: alignment stored as Log32_32(Alignment)+1. I don't quite know what Log32_32 means. Do you mean Log2(Alignment) + 1
Also updated the PNaCl objdumper accordingly. https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:188: // Note: Alignement = 2 ** (Exponent - 1). On 2014/12/16 23:24:39, jvoung wrote: > Alignement -> Alignment Done. https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:189: if (Exponent == 0) { On 2014/12/16 23:24:39, jvoung wrote: > Hmm, I don't think we ever said that "alloca" could not have an align of 0. > > I think only for load/store was that ever checked by the ABI checker. I'm not > sure what C/C++ source would give you an alloca with align 0, but it might > better to continue to allow 0. Is the reason for this the "Exp - 1" vs the "(1 > << x) >> 1" ? Good point. I forgot about the alloca default case. Putting back to use two shifts. https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:193: if (Exponent > 30) { // Note: Exponent is one larger than actual. On 2014/12/16 23:24:39, jvoung wrote: > "than actual" -> "than the limit"? Done. https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:193: if (Exponent > 30) { // Note: Exponent is one larger than actual. On 2014/12/16 23:24:39, jvoung wrote: > include/llvm/IR/Value.h has "MaximumAlignment", which is "29", so you may be > able to use that as the constant. > > The downside of using that is if it changes out from under us. > > Maybe use that + 1, and static assert that it is 29? If upstream changes it, we > could then fork the limit or follow... WDYT? Good point. Adding reference to the constant. https://codereview.chromium.org/807643002/diff/20001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:1549: getLoadStoreAlignmentValue(Record[OpNum], TheModule->getDataLayout(), On 2014/12/16 23:24:39, jvoung wrote: > A little ambivalent about the extra checks -- I don't think it would be easy to > switch the check on here, but switch it off later when the abi checker runs, so > that there isn't new overhead. On the other hand, the overhead is *probably* > negligible. I decided to remove this, since the bitcode reader accepts 0 (for non-finalized pexes), but a finalized pexe applies the additional constraints. Hence, we must wait for the ABI verifier to find the mistake. https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:82: /// Tests if we recognize when alignment gets too large. On 2014/12/16 23:24:39, jvoung wrote: > nit: maybe change Alloc - >"Alloca" to be a bit more specific (vs like malloc > allocation) Done. https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:135: // Show what happens when changing alignment to 30. On 2014/12/16 23:24:39, jvoung wrote: > "to 30" -> "to 2**30" ? Done. https://codereview.chromium.org/807643002/diff/20001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:138: // Note: alignment stored as Log32_32(Alignment)+1. On 2014/12/16 23:24:39, jvoung wrote: > I don't quite know what Log32_32 means. Do you mean Log2(Alignment) + 1 Done.
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( I'm a bit confused about where this is used. https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Reader/... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:201: Alignment = (1 << static_cast<unsigned>(Exponent)) >> 1; It seems like the shift should still happen after checking the exponent? Otherwise Exponent could be something like "67"? https://codereview.chromium.org/807643002/diff/60001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/60001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:235: EXPECT_TRUE(Munger.runTest( Maybe leave a comment that this is returning true because the load/store alignment are only checked later by the ABI checker. However, the DumpMunger does the checking at while parsing.
Clarification for NoDumpFinalizeRules. https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/17 23:36:38, jvoung wrote: > I'm a bit confused about where this is used. One would use this as a command-line argument on a non-finalized pexe. In such cases, the alignment can be zero. However, for finalized pexes, zero is not allowed on load/store. There are actual examples of this where pnacl-freeze allows load/store alignment of 0. It would be nice to be able to run pnacl-bcdis on such files (to see there contents) without getting flooded with error messages. Hence, the point of this flag.
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/17 23:53:00, Karl wrote: > On 2014/12/17 23:36:38, jvoung wrote: > > I'm a bit confused about where this is used. > > One would use this as a command-line argument on a non-finalized pexe. In such > cases, the alignment can be zero. However, for finalized pexes, zero is not > allowed on load/store. > > There are actual examples of this where pnacl-freeze allows load/store alignment > of 0. It would be nice to be able to run pnacl-bcdis on such files (to see there > contents) without getting flooded with error messages. Hence, the point of this > flag. Hmm but does tool work with non-finalized pexes? For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not?
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/18 00:05:40, jvoung wrote: > On 2014/12/17 23:53:00, Karl wrote: > > On 2014/12/17 23:36:38, jvoung wrote: > > > I'm a bit confused about where this is used. > > > > One would use this as a command-line argument on a non-finalized pexe. In such > > cases, the alignment can be zero. However, for finalized pexes, zero is not > > allowed on load/store. > > > > There are actual examples of this where pnacl-freeze allows load/store > alignment > > of 0. It would be nice to be able to run pnacl-bcdis on such files (to see > there > > contents) without getting flooded with error messages. Hence, the point of > this > > flag. > > Hmm but does tool work with non-finalized pexes? > > For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like > bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not? You are correct that we require the overall structure to match PNaCl rules. What I am talking about is that we have many test pexe's (mostly hand written) for which pnacl-freeze works. Note that pnacl-freeze doesn't check that the PNaCl ABI has been fully met. These frozen pexes are then fed into various other tools (using the PNaCl bitcode reader). These tests pass because they don't call the PNaCl ABi verifier. In such cases, since the file is in pexe format, we may want to look at their contents. That is the purpose of this. Note that when I tested the reader restriction on load/store alignments, I discovered this problem. It is one of the reasons I backed off and removed this from the PNaCl bitcode reader. The alternative is to fix (about 15 LLVM tests). I decided not to do this. Rather, I backed out the reader check. Rather, I added a flag to pnacl-bcdis so that they could be read and looked at. An example case of this llvm/test/NaCl/Bitcode/fast.ll. Most of the breakage is the use of 0 alignment for a load/store.
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/18 16:56:05, Karl wrote: > On 2014/12/18 00:05:40, jvoung wrote: > > On 2014/12/17 23:53:00, Karl wrote: > > > On 2014/12/17 23:36:38, jvoung wrote: > > > > I'm a bit confused about where this is used. > > > > > > One would use this as a command-line argument on a non-finalized pexe. In > such > > > cases, the alignment can be zero. However, for finalized pexes, zero is not > > > allowed on load/store. > > > > > > There are actual examples of this where pnacl-freeze allows load/store > > alignment > > > of 0. It would be nice to be able to run pnacl-bcdis on such files (to see > > there > > > contents) without getting flooded with error messages. Hence, the point of > > this > > > flag. > > > > Hmm but does tool work with non-finalized pexes? > > > > For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like > > bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not? > > You are correct that we require the overall structure to match PNaCl rules. What > I am talking about is that we have many test pexe's (mostly hand written) for > which pnacl-freeze works. Note that pnacl-freeze doesn't check that the PNaCl > ABI has been fully met. These frozen pexes are then fed into various other tools > (using the PNaCl bitcode reader). These tests pass because they don't call the > PNaCl ABi verifier. > > In such cases, since the file is in pexe format, we may want to look at their > contents. That is the purpose of this. > > Note that when I tested the reader restriction on load/store alignments, I > discovered this problem. It is one of the reasons I backed off and removed this > from the PNaCl bitcode reader. > > The alternative is to fix (about 15 LLVM tests). I decided not to do this. > Rather, I backed out the reader check. Rather, I added a flag to pnacl-bcdis so > that they could be read and looked at. An example case of this > llvm/test/NaCl/Bitcode/fast.ll. Most of the breakage is the use of 0 alignment > for a load/store. > Okay, I see. Yes it's very easy to do a hand-written test that mistakenly drops the alignment (I've done it several times ;-)). I was confused because the flag isn't used by any test and the default is "false", but it's okay if you know you'll be using it. It feels like for tools that disassemble (like llvm-dis, and pnacl-bcdis) the goal is to see the contents so unless it's not possible it should be able to at least dump the part that has the error. A bit of bikeshedding the name of the flag though. Most of the other flags have "pnaclabi" in them. E.g., "-verify-pnaclabi-functions" or "-pnaclabi-allow-debug-metadata=0", or "-pnaclabi-verify-fatal-errors=0", so it seems better to have something more consistent with those names.
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/18 17:10:22, jvoung wrote: > On 2014/12/18 16:56:05, Karl wrote: > > On 2014/12/18 00:05:40, jvoung wrote: > > > On 2014/12/17 23:53:00, Karl wrote: > > > > On 2014/12/17 23:36:38, jvoung wrote: > > > > > I'm a bit confused about where this is used. > > > > > > > > One would use this as a command-line argument on a non-finalized pexe. In > > such > > > > cases, the alignment can be zero. However, for finalized pexes, zero is > not > > > > allowed on load/store. > > > > > > > > There are actual examples of this where pnacl-freeze allows load/store > > > alignment > > > > of 0. It would be nice to be able to run pnacl-bcdis on such files (to see > > > there > > > > contents) without getting flooded with error messages. Hence, the point of > > > this > > > > flag. > > > > > > Hmm but does tool work with non-finalized pexes? > > > > > > For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like > > > bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not? > > > > You are correct that we require the overall structure to match PNaCl rules. > What > > I am talking about is that we have many test pexe's (mostly hand written) for > > which pnacl-freeze works. Note that pnacl-freeze doesn't check that the PNaCl > > ABI has been fully met. These frozen pexes are then fed into various other > tools > > (using the PNaCl bitcode reader). These tests pass because they don't call the > > PNaCl ABi verifier. > > > > In such cases, since the file is in pexe format, we may want to look at their > > contents. That is the purpose of this. > > > > Note that when I tested the reader restriction on load/store alignments, I > > discovered this problem. It is one of the reasons I backed off and removed > this > > from the PNaCl bitcode reader. > > > > The alternative is to fix (about 15 LLVM tests). I decided not to do this. > > Rather, I backed out the reader check. Rather, I added a flag to pnacl-bcdis > so > > that they could be read and looked at. An example case of this > > llvm/test/NaCl/Bitcode/fast.ll. Most of the breakage is the use of 0 alignment > > for a load/store. > > > > Okay, I see. Yes it's very easy to do a hand-written test that mistakenly drops > the alignment (I've done it several times ;-)). I was confused because the flag > isn't used by any test and the default is "false", but it's okay if you know > you'll be using it. > > It feels like for tools that disassemble (like llvm-dis, and pnacl-bcdis) the > goal is to see the contents so unless it's not possible it should be able to at > least dump the part that has the error. > > A bit of bikeshedding the name of the flag though. Most of the other flags have > "pnaclabi" in them. E.g., "-verify-pnaclabi-functions" or > "-pnaclabi-allow-debug-metadata=0", or "-pnaclabi-verify-fatal-errors=0", so it > seems better to have something more consistent with those names. Another way to look at it is the load/store align values being set to the "right" thing is done by the -pnacl-abi-simplify-{pre/post}opt transformations, not "pnacl-finalize" (which basically does a strip-metadata pass plus pnacl-freeze), so it seems better to not mention "finalize" in this flag name.
https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:43: NoDumpFinalizeRules( On 2014/12/18 17:10:22, jvoung wrote: > On 2014/12/18 16:56:05, Karl wrote: > > On 2014/12/18 00:05:40, jvoung wrote: > > > On 2014/12/17 23:53:00, Karl wrote: > > > > On 2014/12/17 23:36:38, jvoung wrote: > > > > > I'm a bit confused about where this is used. > > > > > > > > One would use this as a command-line argument on a non-finalized pexe. In > > such > > > > cases, the alignment can be zero. However, for finalized pexes, zero is > not > > > > allowed on load/store. > > > > > > > > There are actual examples of this where pnacl-freeze allows load/store > > > alignment > > > > of 0. It would be nice to be able to run pnacl-bcdis on such files (to see > > > there > > > > contents) without getting flooded with error messages. Hence, the point of > > > this > > > > flag. > > > > > > Hmm but does tool work with non-finalized pexes? > > > > > > For example, LLVMBitCodes.h and the plain BitcodeReader.cpp use stuff like > > > bitc::MODULE_CODE_GLOBALVAR, but the NaCl readers do not? > > > > You are correct that we require the overall structure to match PNaCl rules. > What > > I am talking about is that we have many test pexe's (mostly hand written) for > > which pnacl-freeze works. Note that pnacl-freeze doesn't check that the PNaCl > > ABI has been fully met. These frozen pexes are then fed into various other > tools > > (using the PNaCl bitcode reader). These tests pass because they don't call the > > PNaCl ABi verifier. > > > > In such cases, since the file is in pexe format, we may want to look at their > > contents. That is the purpose of this. > > > > Note that when I tested the reader restriction on load/store alignments, I > > discovered this problem. It is one of the reasons I backed off and removed > this > > from the PNaCl bitcode reader. > > > > The alternative is to fix (about 15 LLVM tests). I decided not to do this. > > Rather, I backed out the reader check. Rather, I added a flag to pnacl-bcdis > so > > that they could be read and looked at. An example case of this > > llvm/test/NaCl/Bitcode/fast.ll. Most of the breakage is the use of 0 alignment > > for a load/store. > > > > Okay, I see. Yes it's very easy to do a hand-written test that mistakenly drops > the alignment (I've done it several times ;-)). I was confused because the flag > isn't used by any test and the default is "false", but it's okay if you know > you'll be using it. > > It feels like for tools that disassemble (like llvm-dis, and pnacl-bcdis) the > goal is to see the contents so unless it's not possible it should be able to at > least dump the part that has the error. > > A bit of bikeshedding the name of the flag though. Most of the other flags have > "pnaclabi" in them. E.g., "-verify-pnaclabi-functions" or > "-pnaclabi-allow-debug-metadata=0", or "-pnaclabi-verify-fatal-errors=0", so it > seems better to have something more consistent with those names. Renamed to IgnorePNaClABIChecks (cl flag "-ignore-pnaclabi-checks"). Also added checks on this flag for appropriate calls to "PNaClABI..." methods. https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Reader/... File lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp (right): https://codereview.chromium.org/807643002/diff/60001/lib/Bitcode/NaCl/Reader/... lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp:201: Alignment = (1 << static_cast<unsigned>(Exponent)) >> 1; On 2014/12/17 23:36:38, jvoung wrote: > It seems like the shift should still happen after checking the exponent? > Otherwise Exponent could be something like "67"? Moved after test. https://codereview.chromium.org/807643002/diff/60001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/60001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:235: EXPECT_TRUE(Munger.runTest( On 2014/12/17 23:36:38, jvoung wrote: > Maybe leave a comment that this is returning true because the load/store > alignment are only checked later by the ABI checker. > > However, the DumpMunger does the checking at while parsing. Added comment. Added similar comment to "BadStoreAlignment" below.
otherwise LGTM https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2486: PNaClABITypeChecker::isValidScalarType(BaseTy)) return Op; nit: put "return Op" on a separate line, now that the conditional is > 1 line https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2605: PNaClABITypeChecker::isValidFunctionType(FcnTy)))) { yikes, this one is a bit hard to parse =) If there's any way to simplify it... https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2958: !PNaClABITypeChecker::isValidSwitchConditionType(CondType)) Curlies around the then-statement since it's > 1 line? https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:3285: << *ParamType << "\n"; curlies for > 1 line? https://codereview.chromium.org/807643002/diff/80001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/80001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:237: // pnacl-llc. On the other hand, The DumpMunger checks alignment for ", The " -> ", the "
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 0b3bf5655ca7e4c68107041cbc5d7d8642768df7 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... File lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp (right): https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2486: PNaClABITypeChecker::isValidScalarType(BaseTy)) return Op; On 2014/12/18 19:27:57, jvoung wrote: > nit: put "return Op" on a separate line, now that the conditional is > 1 line Done. https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2605: PNaClABITypeChecker::isValidFunctionType(FcnTy)))) { On 2014/12/18 19:27:57, jvoung wrote: > yikes, this one is a bit hard to parse =) > > If there's any way to simplify it... Done. https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:2958: !PNaClABITypeChecker::isValidSwitchConditionType(CondType)) On 2014/12/18 19:27:57, jvoung wrote: > Curlies around the then-statement since it's > 1 line? Done. https://codereview.chromium.org/807643002/diff/80001/lib/Bitcode/NaCl/Analysi... lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp:3285: << *ParamType << "\n"; On 2014/12/18 19:27:57, jvoung wrote: > curlies for > 1 line? Done. https://codereview.chromium.org/807643002/diff/80001/unittests/Bitcode/NaClPa... File unittests/Bitcode/NaClParseInstsTest.cpp (right): https://codereview.chromium.org/807643002/diff/80001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:237: // pnacl-llc. On the other hand, The DumpMunger checks alignment for On 2014/12/18 19:27:57, jvoung wrote: > ", The " -> ", the " Done. https://codereview.chromium.org/807643002/diff/80001/unittests/Bitcode/NaClPa... unittests/Bitcode/NaClParseInstsTest.cpp:354: // stores while parsing. Here too. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
