inline assembly not supported

[maurelian]

While trying to measure coverage on the ENS, I got this error while instrumenting the Registrar contract:

instrumenting  /Users/primary/Projects/ens-project/security-review/ens-coverage/originalContracts/HashRegistrarSimplified.sol
InlineAssemblyStatement
TypeError: parse[expression.body[x].type] is not a function
    at Object.parseBlockStatement [as BlockStatement] (/Users/primary/Projects/ens-project/security-review/ens-coverage/solcover/parse.js:151:37)

I've hacked around this for now by adding a check to see if the statement type has a parser method.

parse.BlockStatement = function parseBlockStatement(contract, expression) {
  for (let x = 0; x < expression.body.length; x++) {
    instrumenter.instrumentLine(contract, expression.body[x]);
    if (typeof parse[expression.body[x].type] === 'function') {
      try {
        parse[expression.body[x].type](contract, expression.body[x]);
      } catch (e) {
        console.log(expression.body[x].type);
        console.log(e);
      }
    }
  }
};

[maurelian]

I suppose this might be related to the solc version issue as well.

[cgewecke]

Hi @maurelian. Have taken a look at ENS and reproduced your issue, but sadly found several others:

• Parsing inline assembly is now fixed on solcover's truffle3 branch. That branch should also be able to handle solc 0.4.8 contracts and use the latest truffle. There's no coverage for assembler but it parses. (Also, if you use it - check the README because installation is different there).

• solcover is instrumentating this statement in HashRegistrarSimplified.sol incorrectly: Registrar(registrar).acceptRegistrarTransfer(_hash, h.deed, h.registrationDate) That needs to be resolved but I think it can be side stepped by assigning the result of Registrar(registrar) to a temporary variable and using the temp to invoke acceptRegistrarTransfer

• It looks like there might be a bug in solidity-parser with assembler numeric literals. During the instrumentation sequence SP is emitting an error for this line in ReverseRegistrar.sol:

  SyntaxError: Expected comment, end of line, identifier, or whitespace but "0" found. Line: 49, Column: 27

Will verify the SP issue and open a PR over there if possible. I think it will be a couple days before this stuff is straightened out.

[maurelian]

Thanks @cgewecke

I got the same SyntaxError on ReverseRegistrar.sol too. I'm only interested in the HashRegistrarSimplified.sol measurement, so I just commented out the offending section.

I'm now getting the good old Error: Invalid JSON RPC response: "" for reasons I'm not quite sure of. I will probably spend some more time today trying to hack out another work around.

[cgewecke]

Had some luck: here's a report for ENS on codecov.io

Solcover's testrpc instance needed a higher default transaction gas value to run the ENS tests correctly. Also changed a number of ENS things:

• There is a problem resolving a require in one of the migration files while running coverage: commented it out.
• Commented out both ReverseRegistrar.sol and its test (solidity-parser bug)
• Commented out ens_test.js because it tests events - e.g. how many, where in the logs array, etc. Those all fail because the instrumentation corrupts the event values.
• Broke SimpleHashRegistrar's chained constructor/method call on line 558 into two statements
• Commented out the TestRPC provider code in test/util.js and replaced with a line that binds web3 to the modified version of testrpc that solcover has to run.
• Changed all hardcoded gas values in the test files to 0xffffff - (except in test/util.js)
• A couple SimpleHashRegistrar tests that advance time are timing out after 2 seconds. That might resolve if the mocha default was longer.

These changes are implemented on this fork of ENS at the coverage branch.

$ git clone https://github.com/cgewecke/ens.git
$ git checkout coverage
$ npm install
$ npm run solcover

should reproduce the reports.

Ultimately there is a healthy mix of solcover bugs and solcover integration issues here. Gas hardcoding, event testing, and test suites that manage their own relationship to testrpc are problematic for solcover.

[maurelian]

Wow, @cgewecke... you're my one stop code coverage reporting shop. TY for this again.

Ultimately there is a healthy mix of solcover bugs and solcover integration issues here. Gas hardcoding, event testing, and test suites that manage their own relationship to testrpc are problematic for solcover.

One item that nags at me is the dependence on truffle. I use it myself, and love the tool, but it seems like a superficial requirement, that slows development (and possibly adoption) of this tool. To my mind, a code coverage tools could simply:

1. instrument the contracts
2. run the test command (supplied by a config file)
3. watch the events generated.

Has that direction been discussed at all?

[maurelian]

Also, do you know offhand if any of the changes you made would impact the measurement on the HashRegistrarSimplified.sol contract? Reviewing the list it doesn't look like it, but I wasn't sure.

[cgewecke]

@maurelian Thanks so much for opening these issues - it really helps development here - we get to find a ton of bugs and try to iron them out.

I agree with you re: truffle - that seems right to me. Instrumentation and coverage map generation are handled in a modular way within the project so it should be possible to decouple from any given Dapp framework tool. There IS a strong dependency on testrpc though.

@area What's your view of this? In some ways I see truffle as a helpful constraint - it defines a fixed target the tool can be tested against while it's in development.

I'm not sure about the effect of the changes. There are two tests that time out under coverage and I don't know if they just need more time to finish or what. Otherwise everything passes, so it should be the same.

My intuition about mis-measurement is that it will produce false positives rather than conceal problems. It would be great if someone who knew the tests well looked at the coverage reports and could evaluate whether they're right or not.

As a side note - codecov.io 's representation of the report leaves out some of the captured data. If you look at the html report istanbul itself generates for HashRegisterSimplified.sol it shows a number of unreached if statement branches - most of them in modifiers that throw - suggesting that the error cases for those modifiers are never tested. Apart from that there's one untested function and a few other unreached conditional branches. . .

That's all I see. I don't know the code base there well enough to know what anything means.

[maurelian]

As a side note - codecov.io 's representation of the report leaves out some of the captured data. If you look at the html report istanbul itself generates for HashRegisterSimplified.sol it shows a number of unreached if statement branches - most of them in modifiers that throw - suggesting that the error cases for those modifiers are never tested. Apart from that there's one untested function and a few other unreached conditional branches. . .

@cgewecke Unfortunately, I've still been unable to generate the report from that repo. Would you mind sharing the report folder? Whatever method is easiest for you (dropbox, github, etc) is fine.

[cgewecke]

I'm away from my computer for the rest of the day but i'll send it over tomorrow morning.

On Apr 21, 2017 2:37 PM, "maurelian" notifications@github.com wrote:

As a side note - codecov.io 's representation of the report leaves out some of the captured data. If you look at the html report istanbul itself generates for HashRegisterSimplified.sol it shows a number of unreached if statement branches - most of them in modifiers that throw - suggesting that the error cases for those modifiers are never tested. Apart from that there's one untested function and a few other unreached conditional branches. . .

Unfortunately, I've still been unable to generate the report from that repo. Would you mind sharing the report folder? Whatever method is easiest for you (dropbox, github, etc) is fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JoinColony/solcover/issues/59#issuecomment-296311268, or mute the thread https://github.com/notifications/unsubscribe-auth/AG_gugGBO-WKYyMeqP5m7GV2bdgIRnQAks5rySGUgaJpZM4NCQH- .

[cgewecke]

@maurelian The istanbul reports are now included on that ENS fork at the coverage branch. You can also see them with a browser at: https://cgewecke.github.io/ens-coverage-reports/contracts/index.html.

Are you getting an error when you try to clone the ens fork and generate the coverage yourself? If it seems like testrpc is not connecting it's possible that a zombie testrpc instance listening on port 8555 is running in the background on your computer. That can happen when solcover crashes because it uses child_process.exec to launch the server. The only way I've found around it is to restart my computer unfortunately. Going to try to fix that issue shortly . . .

[area]

I agree with you re: truffle - that seems right to me. Instrumentation and coverage map generation are handled in a modular way within the project so it should be possible to decouple from any given Dapp framework tool. There IS a strong dependency on testrpc though.

@area What's your view of this? In some ways I see truffle as a helpful constraint - it defines a fixed target the tool can be tested against while it's in development.

Decoupling from truffle was the intention behind the 'Support for arbitrary testing sommands' item on the original todo list in the Readme, so I agree this should be something that we should include at some point.

Sorry I've been away from Solcover, by the way - work has ramped up the last month or two as we worked towards launching our (closed) beta, which happened yesterday. Hopefully that'll eventually calm down and I can return to working on this in some meaningful capacity, which I rather enjoyed doing...

[area]

Fixed in solidity-coverage.