Page MenuHomePhabricator

RIFTLexer/RIFTParser
Needs ReviewPublic

Authored by mduminer on Nov 26 2019, 3:55 PM.
This revision needs review, but all reviewers have resigned.

Details

Reviewers
jcmcdonald
Maniphest Tasks
T1248: Read from RIFT File
Required Signatures
Restricted Legalpad Document
Summary

Lexing of RIFT bytecode T1259: Tokenize RIFT bytecode

Tokenizes RIFT bytecode for consumption by RIFTParser.

Test Plan

Test suite A-sB10 should pass.

Diff Detail

Repository
rA Anari
Branch
rift_parser
Lint
Lint Passed

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Deserialization of RIFT bytecode T1259: Read from RIFT File
  • Tentative lexer
  • Merge remote-tracking branch 'origin/master' into rift
  • Implementing RIFTByte

Build has FAILED

Building Anari on master as build 51.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/51/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/51/console

Restricted Application failed remote builds in Restricted Buildable!Dec 3 2019, 7:29 PM

Build has FAILED

Building Anari on master as build 53.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/53/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/53/console

Restricted Application failed remote builds in Restricted Buildable!Dec 3 2019, 7:29 PM

Build has FAILED

Building Anari on mpm-bionic-0003jzc7uhrh5 as build 56.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/56/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/56/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 4 2019, 8:27 PM
Restricted Application failed remote builds in Restricted Buildable!

Build has FAILED

Building Anari on mpm-bionic-0004zhv4n0wok as build 59.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/59/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/59/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 6 2019, 12:49 PM
Restricted Application failed remote builds in Restricted Buildable!
  • Refactored RIFTLexer
  • Some refactoring and comments
  • Refactored to use shared_ptr
  • Remove test code

Build has FAILED

Building Anari on mpm-bionic-00061j9tgn72z as build 64.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/64/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/64/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 7 2019, 6:38 PM
Restricted Application failed remote builds in Restricted Buildable!
  • Wrote test
  • Fixed tests
  • removed variant type from bytecode in Token

Build has FAILED

Building Anari on mpm-bionic-0006mlxdez0n1 as build 65.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/65/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/65/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 8 2019, 11:09 AM
Restricted Application failed remote builds in Restricted Buildable!

I haven't found the problem yet, but I'll give you a hint. Ultimately, even if I find the problem, it'll be up to you to completely solve it and fix it. (We learn the most from struggling with a problem.)

I decided to build this with AddressSanitizer, one of the tools in the Clang toolchain. I built it into the Makefiles and CMakeList configuration, so you can build with AddressSanitizer with the following:

make tester_debug SAN=address

Then, you can directly invoke the test you're having trouble with via...

./tester_debug --run A-tB1001

As you probably know from Valgrind, a double free is occuring. This means that a region of memory that was already deallocated previously is being deallocated a second time. This has nothing to do with the destructors themselves. It has everything to do with knowing how certain classes behave.

Both FlexArray and shared_ptr are responsible for allocating their own memory, and for freeing that memory afterward. This makes them exceptionally memory safe. Usually, you wouldn't want to directly use a "raw pointer" in conjunction with them, except in strictly read-only circumstances, and then said pointer should usually itself be const to help prevent accidental modification.

In any case, somewhere in your code — although I haven't quite determined where yet — you are acquiring a pointer or reference to a region of memory already managed by either a FlexArray or a shared_ptr, and giving that to another FlexArray or shared_ptr. As a result, when both go out of scope, their destructors are called, and they try to free a section of memory they somehow share.

This should be correctable. I have no reason to believe the problem is in FlexArray (that was my first suspicion, but no.)

Carefully examine the output from AddressSanitizer: it will give you the full tracebacks for both attempted frees, and the allocation. See if you can find where a region of memory is getting shared.


My AddressSanitizer output is here: P48

anari-source/src/rift_lexer.cpp
69

The main point of defining those RIFTCodes was to use them instead of the hardcoded values. You should only define these hexadecimal values once, in the RIFTCodes enum class, and then use the enum values here.

anari-source/src/test_rift_lexer.cpp
23–50

Typically, setup goes in the pre() function for anything that needs to be run once for any number of calls to run(), or in janitor() for anything that needs to be run before each call to run().

56

While this works, consider using the new PL_ASSERT functions, which are provided in pawlib/pawlib-source/include/pawlib/goldilocks_assertions.hpp. This will provide more useful output by telling you exactly which token failed.

In testing, you always want to ensure you write your tests such that you know exactly what failed any time a failure occurs. You can either do that with the assertion functions mentioned above, or by writing separate tests for each code.

Build has FAILED

Building Anari on mpm-bionic-000a5x8wtcszj as build 67.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/67/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/67/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 12 2019, 2:55 PM
Restricted Application failed remote builds in Restricted Buildable!
mduminer marked an inline comment as done.
  • refactor RIFTLexer to tokenize all the bytecode at once
  • refactor RIFTLexer for reusibility

Build has FAILED

Building Anari on mpm-bionic-000arrgu1v444 as build 68.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/68/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/68/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 13 2019, 8:02 AM
Restricted Application failed remote builds in Restricted Buildable!

Build has FAILED

Building Anari on mpm-bionic-000b4n9dy9da1 as build 69.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/69/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/69/console

Build has FAILED

Building Anari on mpm-bionic-000b4o0spf0a6 as build 70.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/70/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/70/console

  • fixed linter error
  • changed test to use PL_ASSERT

Build has FAILED

Building Anari on mpm-bionic-000cq1dzg8exw as build 74.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/74/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/74/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 15 2019, 3:06 PM
Restricted Application failed remote builds in Restricted Buildable!

Build has FAILED

Building Anari on mpm-bionic-000csbdb6dlpx as build 75.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/75/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/75/console

Restricted Application returned this revision to the author for changes because remote builds failed.Dec 15 2019, 4:53 PM
Restricted Application failed remote builds in Restricted Buildable!
  • squash bug by using make_shared
  • change RIFTLexer::tokenize to use unique_ptr

Build is green

Building Anari on mpm-bionic-000g4asik9ozl as build 79.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/79/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Dec 19 2019, 2:55 PM
  • Fixed lexer's data reading. Fixed bad file reading in test. Removed pointers so FlexArray should work.

Build is green

Building Anari on mpm-bionic-0005t7eyaj3sj as build 85.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/85/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Jan 8 2020, 12:05 PM
  • finished skeleton for RIFTParser

Build has FAILED

Building Anari on mpm-bionic-0006qvsrpo7qu as build 86.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/86/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/86/console

Restricted Application failed remote builds in Restricted Buildable!Jan 9 2020, 2:29 PM
  • fixed comparison of different integer signs

Build is green

Building Anari on mpm-bionic-0006qyx5irf9k as build 87.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/87/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Jan 9 2020, 2:33 PM

Build has FAILED

Building Anari on mpm-bionic-00006q5jkzcnk as build 94.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/94/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/94/console

Restricted Application failed remote builds in Restricted Buildable!Jan 14 2020, 5:00 PM
mduminer edited the test plan for this revision. (Show Details)

Build is green

Building Anari on mpm-bionic-00006u3fgpvi0 as build 95.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/95/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Jan 14 2020, 5:05 PM

Build has FAILED

Building Anari on mpm-bionic-0004gg4tu631i as build 96.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/96/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/96/console

Restricted Application failed remote builds in Restricted Buildable!Jan 19 2020, 5:28 PM

Crazy as it might sound, I actually recommend putting this Differential on hold. Make a fresh new branch off the latest master for StaticLayer:

git checkout master
git pull origin master
git checkout -B staticlayer

Once you finish and land StaticLayer, come back to this branch and Differential and git rebase.

This will need to be taken over by someone later.