Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix writing last bytes in a 16k page of CV data #13

Merged
merged 2 commits into from Jan 3, 2014

Conversation

rainers
Copy link
Contributor

@rainers rainers commented Jan 3, 2014

@denis-sh
Copy link
Member

denis-sh commented Jan 3, 2014

The pull isn't on HEAD. There are lots of commits added after. It solves the problem if applied on parent commit but but currently cv\cvindexc.c is used instead of *.asm file so the pull should be updated.

@rainers
Copy link
Contributor Author

rainers commented Jan 3, 2014

Rebased.

Rant1: it often takes longer to satisfy git than to fix bugs, grrr.

Rant2: I don't believe translating to C in the way it is done in optlink is any good. The code is even worse to read, and it is much harder to find the current definition of a symbol because it often exists 3 times: the original asm, the C code and the commented asm in the C file (you'll have to consult the makefiles to figure out which one is actually used).

WalterBright added a commit that referenced this pull request Jan 3, 2014
fix writing last bytes in a 16k page of CV data
@WalterBright WalterBright merged commit df5f139 into DigitalMars:master Jan 3, 2014
@WalterBright
Copy link
Contributor

Rant2

The C translation is definitely a "work in progress", and so suffers from the issues you mentioned. However, on the plus side, I find over and over that the asm code rarely specifies:

  1. what the registers input to the function are
  2. what the registers output from the function are
  3. the registers that must be preserved by the function

For example, I found a serious bug in the CV code where EDI was being used to pass an argument across multiple layers of nested function calls across modules. There is absolutely zero mention of this in the asm source.

Any work on optlink also suffers terribly from the lack of much of a test suite beyond smoke testing it.

@rainers
Copy link
Contributor Author

rainers commented Jan 6, 2014

Alternatives to improving the underspecification of calling conventions in each function:

  • document them in the asm code
  • use standard calling conventions in asm code, too (e.g. C)

I agree that having the code in C would be a huge gain, but the effort goes to waste if all variables are called ECX and EDI.

Any work on optlink also suffers terribly from the lack of much of a test suite beyond smoke testing it.

How much optlink code do you think is covered by running the dmd test suite? When looking at the list of options probably only a few are ever used.

@WalterBright
Copy link
Contributor

I agree that having the code in C would be a huge gain, but the effort goes to waste if all variables are called ECX and EDI.

Like I said, a work in progress. I don't think it is a waste at all. Converting the asm control flow into structured statements is a large win just for that, for example. Not having to worry about register allocation is another win. Doing calculations with actual expressions is a third win. Separating different uses of the same register into multiple variables with actual types is a fourth.

How much optlink code do you think is covered by running the dmd test suite? When looking at the list of options probably only a few are ever used.

I would guess maybe 50%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants