Skip to content

Conversation

@gareth-rees
Copy link
Member

The object format variant structures (mps_fmt_A_s, mps_fmt_B_s, mps_fmt_auto_header_s) and the functions taking them as arguments (mps_fmt_create_A, mps_fmt_create_B, mps_fmt_create_auto_header) were deprecated in release 1.112.0.

Using the recommended interface mps_fmt_create_k in test cases improves the coverage of that interface, makes it easy to remove the deprecated interfaces if needed, and prepares the test cases for the addition of more format methods if needed.

Additionally, remove copy methods and associated code for counting the number of copies, as copy methods are not called and so this code is unreachable or ineffective.

The object format variant structures (mps_fmt_A_s, mps_fmt_B_s,
mps_fmt_auto_header_s) and the functions taking them as
arguments (mps_fmt_create_A, mps_fmt_create_B,
mps_fmt_create_auto_header) were deprecated in release 1.112.0.

Using the recommended interface mps_fmt_create_k in test cases
improves the coverage of that interfaces, makes it easy to remove the
deprecated interfaces if needed, and prepares the test cases for the
addition of more format methods if needed.

Additionally, remove copy methods and associated code for counting the
number of copies, as copy methods are not called and so this code is
unreachable or ineffective.
@rptb1 rptb1 added test Issue with a test case optional Will cause failures / of benefit. Worth assigning resources. labels Jan 17, 2023
@thejayps
Copy link
Contributor

There is no issue connecting this pull request to customer requirements, however @gareth-rees ' paragraph 2 above explains why this PR is an improvement.

@rptb1
Copy link
Member

rptb1 commented Jan 26, 2023

Reviewing with @UNAA008 and @thejayps . I'm taking the "does it compile and run" role.

On platform.lii6gc:

argerr/146.c:66:17: warning: ‘maxcopy’ defined but not used [-Wunused-variable]
   66 | static long int maxcopy = 0;
      |                 ^~~~~~~
  PASS argerr/146.c (writeable)
argerr/147.c:66:17: warning: ‘maxcopy’ defined but not used [-Wunused-variable]
   66 | static long int maxcopy = 0;
      |                 ^~~~~~~
  PASS argerr/147.c (writeable)
argerr/148.c:66:17: warning: ‘maxcopy’ defined but not used [-Wunused-variable]
   66 | static long int maxcopy = 0;
      |                 ^~~~~~~
...
function/97.c: In function ‘area_scan’:
function/97.c:121:18: warning: unused variable ‘q’ [-Wunused-variable]
  121 |    mps_addr_t p, q;
      |                  ^
  PASS function/97.c (writeable)
  PASS function/98.c (writeable)
  PASS function/99.c (writeable)
  PASS function/100.c (writeable)
  PASS function/101.c (writeable)
  PASS function/103.c (writeable)
function/104.c: In function ‘area_scan’:
function/104.c:123:18: warning: unused variable ‘q’ [-Wunused-variable]
  123 |    mps_addr_t p, q;
      |                  ^
  PASS function/104.c (writeable)
  PASS function/105.c (writeable)
function/106.c: In function ‘test’:
function/106.c:81:7: warning: implicit declaration of function ‘make_format_header’ [-Wimplicit-function-declaration]
   81 |  cdie(make_format_header(&format, arena), "create format");
      |       ^~~~~~~~~~~~~~~~~~
function/106.c:81:7: warning: nested extern declaration of ‘make_format_header’ [-Wnested-externs]
/usr/bin/ld: /tmp/ccTqOD0m.o: in function `test':
/home/rb/git/mps/test/function/106.c:81: undefined reference to `make_format_header'
collect2: error: ld returned 1 exit status
...
X FAIL function/106.c (writeable)
X FAIL function/136.c (writeable)

On plaform.lii6ll:

function/106.c:81:7: warning: implicit declaration of function 'make_format_header'
      [-Wimplicit-function-declaration]
 cdie(make_format_header(&format, arena), "create format");
      ^
1 warning generated.
/usr/bin/ld: /tmp/106-ca8794.o: in function `test':
106.c:(.text+0xbc): undefined reference to `make_format_header'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
X FAIL function/106.c (writeable)
X FAIL function/136.c (writeable)

Some of these problems are not to do with this branch and pull request, but are present in master and shown up by work on #116 . But one of these at least is clearly local to this branch.

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for deleting fmtno.c and fmtno.h?

Copy link
Member

@rptb1 rptb1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails to build and link. See #85 (comment) .

Introduces numerous test failures on Windows. See https://github.com/Ravenbrook/mps/actions/runs/4016780980/jobs/6900268157#step:3:459

These need diagnosing and this branch needs revising.

@rptb1
Copy link
Member

rptb1 commented Jan 26, 2023

Btw, this illustrates why I have been concentrating on CI, merge procedures, and review, to make up for lack of coverage and check these recent PRs thoroughly before merging.

die(mps_thread_reg(&thread, arena), "register thread");
cdie(mps_root_create_thread(&root, arena, thread, stack_pointer), "thread root");
die(mps_fmt_create_A(&format, arena, &fmtLO), "create format");
cdie(make_format_header(&format, arena), "create format");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

105.c and 107.c transformed mps_fmt_create_A into make_format, but this uses make_format_header, and fails to compile. Format A doesn't have a header field, so this is probably a mistake. Can you confirm @gareth-rees ?

@rptb1
Copy link
Member

rptb1 commented Jan 26, 2023

X FAIL function/136.c (writeable)

This is failing on master. See https://github.com/Ravenbrook/mps/actions/runs/3939501021/jobs/6739430364#step:6:865 . No action needed in this branch.

@rptb1
Copy link
Member

rptb1 commented Jan 26, 2023

Introduces numerous test failures on Windows. See https://github.com/Ravenbrook/mps/actions/runs/4016780980/jobs/6900268157#step:3:459

I'm stopping work now but I have a suspicion that this might be to do with the difference in MPS_PF_ALIGN (see mpstd.h) on Windows and Linux. If the format specifies no alignment, Windows will align to 16 bytes

#define MPS_PF_ALIGN 16

but Linux to 8

#define MPS_PF_ALIGN 8

When e.g. the Dylan objects are created with alignment 8, the MPS will complain on Windows and not on Linux. So perhaps somewhere an alignment has been dropped.

There's another issue that we ought to have a good explanation of why these alignments vary by platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optional Will cause failures / of benefit. Worth assigning resources. test Issue with a test case

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants