Ticket #1105 (closed patch: wontfix)

Opened 5 years ago

Last modified 5 years ago

change to a libjit based frame builder

Reported by: plobsing Owned by: jkeenan
Priority: normal Milestone:
Component: core Version: branch
Severity: medium Keywords:
Cc: jkeenan, whiteknight, bacek Language:
Patch status: applied Platform:

Description (last modified by darbelo) (diff)

This patch removes the old i386 frame builder and replaces it with one based on libjit.

Changes:

remove all but the public API from public files:

src/frame_builder.h src/frame_builder.c

make jit_buffer_private_data truly private:

tools/build/nativecall.pl

add test of dynamic frame builder:

src/nci_test.c t/pmc/nci.t

detect presence of libjit:

config/auto/libjit.pm config/auto/libjit config/auto/libjit/libjit_c.in t/steps/auto/libjit-01.t

generate source files:

config/gen/libjit config/gen/libjit/frame_builder_libjit_c.in config/gen/libjit/frame_builder_libjit_h.in t/steps/gen/libjit-01.t

add new steps, as well as '--without-libjit' option, to Configure.pl:

lib/Parrot/Configure/Options/Conf.pm lib/Parrot/Configure/Options/Conf/Shared.pm lib/Parrot/Configure/Step/List.pm

add new source files to build:

config/gen/makefiles/root.in

Attachments

libjit.patch Download (107.8 KB) - added by plobsing 5 years ago.
20091013.41837.auto_libjit.txt.gz Download (5.0 KB) - added by jkeenan 5 years ago.
'make' output in auto_libjit branch, failing at line 505
nolibjit_ifdef_fix.patch Download (0.8 KB) - added by plobsing 5 years ago.
quickfix_makefile_deps.patch Download (0.8 KB) - added by plobsing 5 years ago.
fix_auto_frames_detection.patch Download (508 bytes) - added by plobsing 5 years ago.
remove_generated_files.patch Download (33.1 KB) - added by plobsing 5 years ago.
alloca_optional.patch Download (17.9 KB) - added by plobsing 5 years ago.
modified.alloca_optional.patch Download (22.0 KB) - added by jkeenan 5 years ago.
What actually got applied to branch
libjit_doc.patch Download (0.9 KB) - added by plobsing 5 years ago.
small patch to add documentation describing libjit
config_fixup.patch Download (13.1 KB) - added by plobsing 5 years ago.
remove exec protect checking from config::auto::frames, remove frame building checking from config::auto::libjit, small codingstd fixes
codingstd_outdent.patch Download (1.7 KB) - added by plobsing 5 years ago.
proposed change to codingstd/c_indent.t to better reflect pdd07
pcc_reapply_merge_fixups.patch Download (74.4 KB) - added by plobsing 5 years ago.
fixups after merging trunk
pre_merge.patch Download (29.6 KB) - added by plobsing 5 years ago.
patches from TT#1147 applied to trunk
merge.patch Download (139.9 KB) - added by plobsing 5 years ago.
auto_libjit, patched (TT1147) trunk merge
post_merge.patch Download (74.4 KB) - added by plobsing 5 years ago.
fixups on new branch after merger
merge_complete.patch Download (172.6 KB) - added by plobsing 5 years ago.
pre_merge, merge, and post_merge patches combined
merge_complete_tweaked.patch Download (161.0 KB) - added by plobsing 5 years ago.
slightly modified merge patch (now with 100% less FAIL!)

Change History

Changed 5 years ago by plobsing

follow-ups: ↓ 2 ↓ 3   Changed 5 years ago by jkeenan

plobsing,

This patch is a very fine effort and obviously reflects many hours of work. Thanks for taking the time to prepare it, including the time you clearly spent reading the documentation on how to add configuration steps, how to add command-line options for Configure.pl, how to test the configuration steps, etc. And all that before we even get to the meat of the patch!

I created  the auto_libjit branch in SVN mainly so that I could run this through various parts of the test suite. I have a couple of comments, which happen to occur in increasing order of importance:

1. I have a hunch that auto::libjit should be moved earlier in the list of configuration steps, perhaps just before auto::jit. But at this point that's only a hunch.

2. gen::libjit has to be moved earlier. gen::makefiles, gen::platform and gen::config_pm must be the three final configuration steps. But I cannot yet say exactly where in the gen category it should go. For testing purposes, I have moved it to the 4th last step, i.e., just before gen::makefiles.

3. When running perl Configure.pl --verbose-step=auto::libjit --fatal-step=auto::libjit on Linux/i386 -- where libjit is probably not yet installed -- auto::libjit failed with the following output:

auto::libjit -        Is LibJIT installed...
cc  -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -DHASATTRIBUTE_CONST  -DHASATTRIBUTE_DEPRECATED  -DHASATTRIBUTE_MALLOC  -DHASATTRIBUTE_NONNULL  -DHASATTRIBUTE_NORETURN  -DHASATTRIBUTE_PURE  -DHASATTRIBUTE_UNUSED  -DHASATTRIBUTE_WARN_UNUSED_RESULT  -falign-functions=16 -fvisibility=hidden -funit-at-a-time -maccumulate-outgoing-args -W -Wall -Waggregate-return -Wcast-align -Wcast-qual -Wchar-subscripts -Wcomment -Wdisabled-optimization -Wendif-labels -Wextra -Wformat -Wformat-extra-args -Wformat-nonliteral -Wformat-security -Wformat-y2k -Wimplicit -Wimport -Winit-self -Winline -Winvalid-pch -Wlogical-op -Wmissing-braces -Wmissing-field-initializers -Wno-missing-format-attribute -Wmissing-include-dirs -Wpacked -Wparentheses -Wpointer-arith -Wreturn-type -Wsequence-point -Wno-shadow -Wsign-compare -Wstrict-aliasing -Wstrict-aliasing=2 -Wswitch -Wswitch-default -Wtrigraphs -Wundef -Wunknown-pragmas -Wno-unused -Wvariadic-macros -Wwrite-strings -Wbad-function-cast -Wc++-compat -Wdeclaration-after-statement -Werror=declaration-after-statement -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wnonnull -DHAS_GETTEXT  -I./include -c test_29572.c
test_29572.c:14:21: error: jit/jit.h: No such file or directory
test_29572.c: In function 'main':
test_29572.c:18: warning: implicit declaration of function 'jit_init'
test_29572.c:18: warning: nested extern declaration of 'jit_init'
test_29572.c:19: warning: implicit declaration of function 'jit_uses_interpreter'
test_29572.c:19: warning: nested extern declaration of 'jit_uses_interpreter'

step auto::libjit died during execution: C compiler failed (see test_29572.cco) at lib/Parrot/Configure/Compiler.pm line 107
        Parrot::Configure::Compiler::cc_build('Parrot::Configure=HASH(0x8307a90)', '', '-ljit') called at config/auto/libjit.pm line 56
        eval {...} called at config/auto/libjit.pm line 56
        auto::libjit::runstep('auto::libjit=HASH(0x8604870)', 'Parrot::Configure=HASH(0x8307a90)') called at lib/Parrot/Configure.pm line 412
        eval {...} called at lib/Parrot/Configure.pm line 412
        Parrot::Configure::_run_this_step('Parrot::Configure=HASH(0x8307a90)', 'HASH(0x8494568)') called at lib/Parrot/Configure.pm line 267
        Parrot::Configure::runsteps('Parrot::Configure=HASH(0x8307a90)') called at Configure.pl line 75

 at Configure.pl line 75

Thank you very much.

kid51

in reply to: ↑ 1   Changed 5 years ago by jkeenan

I applied the patch to branch in r41837. I added SVN properties and rebuilt the MANIFEST.

in reply to: ↑ 1   Changed 5 years ago by jkeenan

Replying to jkeenan: Given this output in the error file:

test_29572.c:14:21: error: jit/jit.h: No such file or directory
test_29572.c: In function 'main':
test_29572.c:18: warning: implicit declaration of function 'jit_init'
test_29572.c:18: warning: nested extern declaration of 'jit_init'
test_29572.c:19: warning: implicit declaration of function 'jit_uses_interpreter'
test_29572.c:19: warning: nested extern declaration of 'jit_uses_interpreter'

... I suspect what's happening is that the C compiler is searching for jit/jit.h, failing (correctly, on this box) to find it, and then failing non-gracefully. Is there some sort of #ifndef we could write to say, "If you don't find jit/jit.h, don't try any further to compile, just print a message that runstep will recognize as failure and exit"?

kid51

  Changed 5 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned

  Changed 5 years ago by doughera

On Tue, 13 Oct 2009, Parrot wrote:

>  Replying to [comment:1 jkeenan]:
>  Given this output in the error file:
>  {{{
>  test_29572.c:14:21: error: jit/jit.h: No such file or directory
>  test_29572.c: In function 'main':
>  test_29572.c:18: warning: implicit declaration of function 'jit_init'
>  test_29572.c:18: warning: nested extern declaration of 'jit_init'
>  test_29572.c:19: warning: implicit declaration of function
>  'jit_uses_interpreter'
>  test_29572.c:19: warning: nested extern declaration of
>  'jit_uses_interpreter'
>  }}}
>  ... I suspect what's happening is that the C compiler is searching for
>  ''jit/jit.h'', failing (correctly, on this box) to find it, and then
>  failing non-gracefully.  Is there some sort of `#ifndef` we could write to
>  say, "If you don't find ''jit/jit.h'', don't try any further to compile,
>  just print a message that `runstep` will recognize as failure and exit"?

I suspect that the most sensible thing to do is simply remove the 'die' 
command from the probe if the compilation fails, something like

Index: config/auto/libjit.pm
===================================================================
--- config/auto/libjit.pm	(revision 41839)
+++ config/auto/libjit.pm	(working copy)
@@ -58,8 +58,6 @@
     if (!$@) {
         my $test = $conf->cc_run();
         $has_libjit = $self->_evaluate_cc_run($conf, $test, $has_libjit, $verbose);
-    } else {
-        die $@;
     }
 
     $conf->data->set( HAS_LIBJIT => $has_libjit );


-- 
    Andy Dougherty		doughera@lafayette.edu

  Changed 5 years ago by jkeenan

Andy's suggestion led me to refactor this part of runstep(), with the result that the config step is now completing successfully even when the C probe functions do not succeed. Cf. r41849, in which I also moved auto::libjit to the position immediately preceding auto::jit.

follow-up: ↓ 8   Changed 5 years ago by jkeenan

While Configure.pl now completes in the auto_libjit branch, I got a make failure when building on Linux/i386. Complete make output is attached; here are the most relevant lines:

335 src/frame_builder.c
336 src/frame_builder_libjit.c
337 In file included from src/frame_builder_libjit.c:20:
338 src/frame_builder_libjit.h:26:5: warning: "PARROT_HAS_LIBJIT" is not defined
339 src/frame_builder_libjit.c:22:5: warning: "PARROT_HAS_LIBJIT" is not defined
...
470 compilers/imcc/imcparser.c: In function 'yyparse':
471 compilers/imcc/imcparser.c:3092: warning: statement with no effect
472 compilers/imcc/imcparser.c:5225: warning: logical '&&' with non-zero constant w    ill always evaluate as true
473 compilers/imcc/imcparser.c:5228: warning: statement with no effect
474 compilers/imcc/imcparser.c:5385: warning: statement with no effect
475 compilers/imcc/imcparser.c:5389: warning: statement with no effect
476 compilers/imcc/imcc.y:1360: warning: ignoring return value of 'iSUBROUTINE', de    clared with attribute warn_unused_result
...
496 ( cd blib/lib ; ln -sf libparrot.so.1.6.0 libparrot.so )
497 src/main.c
498 /usr/local/bin/perl tools/build/parrot_config_c.pl --mini > \
499     src/null_config.c
500 src/null_config.c
501 cc -o miniparrot src/main.o src/null_config.o \
502     -Wl,-rpath=/home/jimk/work/auto_libjit/blib/lib -L/home/jimk/work/auto_libj    it/blib/lib -lparrot -lpthread -lm -L/usr/lib  -licuuc -licudata -lpthread -lm     -lnsl -ldl -lm -lcrypt -lutil -lpthread -lrt -lgmp -lreadline  -fstack-protecto    r -L/usr/local/lib -Wl,-E
503 /home/jimk/work/auto_libjit/blib/lib/libparrot.so: undefined reference to `Parr    ot_jit_build_call_func'
504 collect2: ld returned 1 exit status
505 make: *** [miniparrot] Error 1

For comparison purposes, 'normal' make output on this box is approx 795 lines long. As you can see, this build died at line 505.

Thank you very much.

kid51

Changed 5 years ago by jkeenan

'make' output in auto_libjit branch, failing at line 505

in reply to: ↑ 7 ; follow-up: ↓ 9   Changed 5 years ago by plobsing

Replying to jkeenan:

Sorry about the lack of testing on boxes that lack libjit.

It appears that had some #ifs that should have been #ifdefs. After the change, 'perl Configure.pl; make test' succeeds on my dev box (I uninstalled libjit for the test).

Changed 5 years ago by plobsing

in reply to: ↑ 8 ; follow-up: ↓ 10   Changed 5 years ago by plobsing

Replying to plobsing:

Actually the above changes (while fixing legitimate bugs) will probably not work on x86. It seems you've uncovered one place this patch missed: config/gen/frames.pm. It still decides whether to use static thunks or a framebuilder based on the capabilities of the old frame builder.

Also in testing, I found that reconfiguring while toggling HAS_LIBJIT does not trigger a rebuild of frame_builder_libjit$(O) as it should. The quick fix is to change the makefile header dependancies back to $(GENERAL_H_FILES).

Changed 5 years ago by plobsing

Changed 5 years ago by plobsing

in reply to: ↑ 9 ; follow-up: ↓ 12   Changed 5 years ago by jkeenan

Replying to plobsing:

Sorry I have not gotten back to you. (For some reason, even though I am the "Owner" of this ticket, I got no email updates from your posts. I guess I'll add myself as a "cc" to make sure.)

I applied the three patches to the auto_libjit branch in the order in which you posted them. I then ran: perl Configure.pl --test --configure_trace. I got a failure in t/steps/auto/frames-01.t, which I am now trying to debug.

t/steps/auto/frames-01.t .................... 1/23 
#   Failed test 'Result is 'yes', as expected'
#   at t/steps/auto/frames-01.t line 47.
#          got: 'no'
#     expected: 'yes'

#   Failed test '_call_frames_buildable() returned true value, as expected (i386/non darwin/8)'
#   at t/steps/auto/frames-01.t line 72.
# Looks like you failed 2 tests of 23.
t/steps/auto/frames-01.t .................... Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/23 subtests 

Thank you very much.

kid51

  Changed 5 years ago by jkeenan

  • cc jkeenan added

in reply to: ↑ 10 ; follow-ups: ↓ 13 ↓ 17   Changed 5 years ago by jkeenan

Replying to jkeenan:

Replying to plobsing: I applied the three patches to the auto_libjit branch in the order in which you posted them. I then ran: perl Configure.pl --test --configure_trace. I got a failure in t/steps/auto/frames-01.t, which I am now trying to debug.

Reverting the last of your three patches -- which modified config/auto/frames.pm -- enabled t/steps/auto/frames-01.t to pass once again. This is not entirely surprising to me, because I know that lines 52-53 in that file were carefully chosen by bacek to reflect what we knew about the framebuilding capabilities of different OS/platform combinations. So I wrote tests in t/steps/auto/frames-01.t to reflect that.

Now, the current formulation of config/auto/frames.pm is almost certainly not the last word in detecting framebuilding. So your approach may ultimately prove correct. I don't know enough myself to judge one way or the other. And, of course, it may simply be the test file that needs revising (but I'm skeptical that that's the only problem).

For better or worse, when I then tried make, I got the same build failure as before. The output of my make log at r41891 differed from that which I posted for r41837 only in the following:

$ diff -w 20091013.41837.auto_libjit.txt \
> 20091016.41891.auto_libjit.txt 
337,339d336
< In file included from src/frame_builder_libjit.c:20:
< src/frame_builder_libjit.h:26:5: warning: "PARROT_HAS_LIBJIT" is not defined
< src/frame_builder_libjit.c:22:5: warning: "PARROT_HAS_LIBJIT" is not defined

So we've eliminated two warnings during make, but have not yet gotten make to complete on a Linux/i386 box where libjit is not installed.

I committed to the branch the first two patches: see r41892. Thank you for your continued efforts in this.

kid51

in reply to: ↑ 12 ; follow-ups: ↓ 15 ↓ 16   Changed 5 years ago by jkeenan

  • cc whiteknight, bacek added

Replying to jkeenan:

I committed to the branch the first two patches: see r41892. Thank you for your continued efforts in this.

I re-tried plobsing's third patch, this time without worrying about whether t/steps/auto/frames-01.t would pass or not. I am pleased to report that with that patch Parrot built and passed all tests in make test. So I applied it to the branch in r41893. I got the same good results on both Linux/i386 and Darwin/PPC.

I will take a look at revising t/steps/auto/frames-01.t so that it reflects the new code in config/auto/frames.pm. However, before we merge this to trunk we'll have to look carefully at the differences in auto::frames between this branch and trunk. For that purpose, I'm cc-ing whiteknight and bacek, though I'm aware that both are probably busy with pcc_reapply at the moment.

Thank you very much.

kid51

  Changed 5 years ago by jkeenan

  • summary changed from [PATCH] change to a libjit based frame builder to change to a libjit based frame builder
  • version changed from trunk to branch
  • patch set to applied

in reply to: ↑ 13   Changed 5 years ago by jkeenan

Replying to jkeenan:

I am pleased to report that with that patch Parrot built and passed all tests in make test. So I applied it to the branch in r41893. I got the same good results on both Linux/i386 and Darwin/PPC.

Here is a  Smolder report on the branch run on Linux/i386.

in reply to: ↑ 13   Changed 5 years ago by jkeenan

Replying to jkeenan:

I will take a look at revising t/steps/auto/frames-01.t so that it reflects the new code in config/auto/frames.pm.

I got it working again in r41894. I also saw three statements in one of the internal methods that were no longer needed, so I deleted them.

kid51

in reply to: ↑ 12 ; follow-up: ↓ 18   Changed 5 years ago by plobsing

Replying to jkeenan:

I committed to the branch the first two patches: see r41892.

It appears that src/frame_builder_libjit.{h,c} got added to version control and the manifest in this changeset. This is likely an error as they are generated (in config/gen/libjit.pm).

Changed 5 years ago by plobsing

in reply to: ↑ 17   Changed 5 years ago by jkeenan

Replying to plobsing:

Replying to jkeenan:

I committed to the branch the first two patches: see r41892.

It appears that src/frame_builder_libjit.{h,c} got added to version control and the manifest in this changeset. This is likely an error as they are generated (in config/gen/libjit.pm).

Yes, I knew I was getting confused there. However, IIRC, the better solution is to add them to MANIFEST.generated rather than MANIFEST.SKIP. I'm assuming that Parrot and HLL developers would want these available to them, hence inclusion in generated. I also made sure they would get added to MANIFEST.configure.generated.

Please see r41899.

Thank you very much,

kid51

follow-up: ↓ 20   Changed 5 years ago by jkeenan

I've done some refactoring of auto::libjit::runstep() to make it more amenable to testing, eliminate unneeded variables and reduce the number of return points to just one. I've added tests in t/steps/auto/libjit-01.t, with the result that the test coverage is about as far as it's going to get on a box (like the one I use for this purpose) where libJIT is not yet installed. (See  coverage report).

Two points:

1. In the DESCRIPTION section of config/auto/libjit.pm, it would be good to have some documentation as to what libJIT is, where its documentation can be found and how to download and install it.

2. In auto::libjit::_handle_has_libjit() we set has_exec_protect to a true value in the Parrot::Configure object if the system has libJIT. But two configuration steps later, in auto::frames, we have other code which sets has_exec_protect:

 62         if ( -e "config/auto/frames/test_exec_${osname}_c.in" ) {
 63             $conf->cc_gen("config/auto/frames/test_exec_${osname}_c.in");
 64             eval { $conf->cc_build(); };
 65             if ($@) {
 66                 $conf->data->set( has_exec_protect => 0 );
 67             }
 68             else {
 69                 my $exec_protect_test = (
 70                     $conf->cc_run(0) !~ /ok/ && $conf->cc_run(1) =~ /ok/
 71                 );
 72                 if ($exec_protect_test) {
 73                     $conf->data->set( has_exec_protect => 1 );
 74                 }
 75                 else {
 76                     $conf->data->set( has_exec_protect => 0 );
 77                 }
 78             }
 79             $conf->cc_clean();
 80         }
 81         else {
 82             $conf->data->set( has_exec_protect => 0 );
 83         }
 84         $self->set_result( 'yes' );
 85     }

It doesn't seem right to make a determination of a Parrot::Configure element in one step only to re-set it two configuration steps later. Can anyone shed light on this issue?

Thank you very much.

kid51

in reply to: ↑ 19 ; follow-up: ↓ 21   Changed 5 years ago by plobsing

Replying to jkeenan:

It doesn't seem right to make a determination of a Parrot::Configure element in one step only to re-set it two configuration steps later. Can anyone shed light on this issue?

That worked when the libjit steps were the last of their types because the config::gen::libjit settings would override the config::gen::frames settings.

The root problem is this: PARROT_HAS_EXEC_PROTECT is a misnomer. It should be set whenever the JIT subsystem has to manage its own executable memory (which is likely always with external JIT systems). In fact, the small gain in an edge case (using a homebrew jit, which we no longer have) is probably not worth the confusion this flag will cause as we add JIT systems. I hesitate to remove it as it increases the scope of this patch.

A compounding problem is that the test suite does not exercise JIT buffer cloning and has very little testing of JIT buffer destroy. It also will likely not catch memory leaks created when the JIT subsystem memory management routines are not being called.

in reply to: ↑ 20 ; follow-up: ↓ 22   Changed 5 years ago by plobsing

Replying to plobsing:

With the following patch, the frame builder should work on platforms that don't have alloca support in libjit (ie everything but i386). This comes at a cost of one malloc/free per invocation.

This also gave the opportunity to remove some code and simplify other code.

Changed 5 years ago by plobsing

in reply to: ↑ 21 ; follow-up: ↓ 23   Changed 5 years ago by jkeenan

Replying to plobsing:

Replying to plobsing: With the following patch, the frame builder should work on platforms that don't have alloca support in libjit (ie everything but i386).

I am in the process of applying this patch in the auto_libjit branch. However, when I run perl Configure.pl --test=configure, I am getting "uninitialized value" warnings in t/steps/auto/libjit-01.t and t/steps/gen/libjit-01.t. In the first of these, at least, this was a side effect of the addition of the ternary condition in config/auto/libjit. I am working on fixing this, so repairs to the tests will go in to branch at the same time as the rest of plobsing's patch.

Thank you very much.

kid51

in reply to: ↑ 22 ; follow-up: ↓ 24   Changed 5 years ago by jkeenan

Replying to jkeenan:

Replying to plobsing:

Replying to plobsing:

With modifications not yet committed, I'm now running the preconfiguration tests successfully and without warnings. But I'm getting this error during perl Configure.pl itself:

gen::libjit -         Generate LibJIT specific code...value for 'libjit_has_alloca' in config/gen/libjit/frame_builder_libjit_h.in is undef at lib/Parrot/Configure/Compiler.pm line 547, <$in> line 66.
...................done.

This is on a Linux/i386 box where libJIT is not installed. Continuing to investigate.

Thank you very much.

kid51

in reply to: ↑ 23   Changed 5 years ago by jkeenan

Replying to jkeenan:

Replying to jkeenan:

Replying to plobsing:

Replying to plobsing:

See attachment modified.alloca_optional.patch to see what was committed to branch in r42003. This configures cleanly, builds and passes all tests in make test. The principal difference between plopsing's original patch and what got committed are these lines in config/auto/libjit.pm:

31a139,144
> +    else {
> +        $conf->data->set( libjit_has_alloca => 0 );
> +    }
>  }

This guarantees that libjit_has_alloca has a defined value on any platform. This averts an uninitialized value error in config/gen/libjit.pm.

 Here is a Smolder report on the auto_libjit branch. No libJit on the box used for testing.

Thank you very much.

kid51

Changed 5 years ago by jkeenan

What actually got applied to branch

Changed 5 years ago by plobsing

small patch to add documentation describing libjit

follow-up: ↓ 26   Changed 5 years ago by jkeenan

In r42008, r42009, r42010, r42011 and r42012, I have corrected coding standards errors. But there are still some errors in C files.

plobsing, can you run:

perl t/codingstd/c_header_guards.t perl t/codingstd/c_indent.t perl t/codingstd/c_macro_args.t

... and correct as needed?

Thank you very much.

kid51

in reply to: ↑ 25 ; follow-up: ↓ 27   Changed 5 years ago by plobsing

Replying to jkeenan:

In r42008, r42009, r42010, r42011 and r42012, I have corrected coding standards errors. But there are still some errors in C files. plobsing, can you run: perl t/codingstd/c_header_guards.t perl t/codingstd/c_indent.t perl t/codingstd/c_macro_args.t ... and correct as needed? Thank you very much. kid51

Thank you for fixing these and pointing the others out.

I have corrected the errors flagged by c_header_guards.t and c_macro_args.t in my last patch.

However, my violations of c_indent.t comply with PDD07. Specifically: 'Labels (including case labels) must be outdented two columns relative to the code they label.'

Changed 5 years ago by plobsing

remove exec protect checking from config::auto::frames, remove frame building checking from config::auto::libjit, small codingstd fixes

Changed 5 years ago by plobsing

proposed change to codingstd/c_indent.t to better reflect pdd07

in reply to: ↑ 26 ; follow-up: ↓ 28   Changed 5 years ago by jkeenan

Replying to plobsing:

I have corrected the errors flagged by c_header_guards.t and c_macro_args.t in my last patch. However, my violations of c_indent.t comply with PDD07. Specifically: 'Labels (including case labels) must be outdented two columns relative to the code they label.'

Thanks for the patches. I will try to get these patches either tonight or by next Tuesday. (Others are free to work on them as well, of course!)

kid51

in reply to: ↑ 27   Changed 5 years ago by jkeenan

Applied the config_fixup.patch in r42030. See  this Smolder report done after testing but before the commit.

Discussed the c_indent codingstd patch with plobsing on #parrot. Recommended that it be submitted in a separate ticket, as it affects a lot more files than those concerned with libjit. We'll worry about failures to pass that test in the auto_libjit branch next week.

Thank you very much.

kid51

  Changed 5 years ago by plobsing

Some updates to auto_libjit:

  • merge from trunk after pcc_reapply (and after applying patches from TT1147)
  • fix errors generated by merge

After these, it seems to work. It has an error in t/src/warnings.t if configured with --optimize, but I also get this in trunk.

Changed 5 years ago by plobsing

fixups after merging trunk

Changed 5 years ago by plobsing

patches from TT#1147 applied to trunk

Changed 5 years ago by plobsing

auto_libjit, patched (TT1147) trunk merge

Changed 5 years ago by plobsing

fixups on new branch after merger

  Changed 5 years ago by plobsing

merging trunk into auto_libjit after pcc_reapply generated a patch that was too large.

New strategy:

  • create a new branch from trunk
  • apply patches from TT#1147 (pre_merge.patch)
  • merge auto_libjit into new branch (merge.patch)
  • fix errors in new branch (post_merge.patch)

This generates a new branch that 1) has pcc_reapply, 2) has libjit work, 3) passes make test.

Changed 5 years ago by plobsing

pre_merge, merge, and post_merge patches combined

  Changed 5 years ago by plobsing

darbelo++ tried commiting the pre_merge, merge, and post_merge patches to a new branch. Unfortunately it got messed up somehow. Maybe one slightly bigger patch combining all three would be easier. Hence merge_complete.patch.

follow-up: ↓ 33   Changed 5 years ago by plobsing

and that patch won't work either :-S. attaching a slightly hand-modified, tested patch. It doesn't apply cleanly, but it does pass tests.

I can't figure out how to tell diff/patch that I really do want to delete files so, for the following files, 1) no this isn't a reversed patch 2) these files also *should* be deleted:

  • src/frame_builder.c
  • config/auto/frames/*

Also don't worry about src/frame_builder.h not applying cleanly. It applies clean enough.

Changed 5 years ago by plobsing

slightly modified merge patch (now with 100% less FAIL!)

in reply to: ↑ 32   Changed 5 years ago by darbelo

  • description modified (diff)

Replying to plobsing:

and that patch won't work either :-S. attaching a slightly hand-modified, tested patch. It doesn't apply cleanly, but it does pass tests. I can't figure out how to tell diff/patch that I really do want to delete files so, for the following files, 1) no this isn't a reversed patch 2) these files also *should* be deleted: * src/frame_builder.c * config/auto/frames/* Also don't worry about src/frame_builder.h not applying cleanly. It applies clean enough.

follow-up: ↓ 35   Changed 5 years ago by darbelo

  • description modified (diff)

Ugh. changed the description when I meant to reply. Fixing.

Latest patch applied to the new libjit_framebuilder branch, passes all tests on OpenBSD i386 without libjit installed. I don't have easy to install libjit packages for my platform, so all I'm able to check right now is that it introduces no regressions.

Somebody on a libjit-enabled platform should test (and benchmark) the branch as well before we merge back into trunk. But the code looks good overall and I consider this a step in the right direction.

in reply to: ↑ 34   Changed 5 years ago by jkeenan

Replying to darbelo:

Latest patch applied to the new libjit_framebuilder branch, passes all tests on OpenBSD i386 without libjit installed.

darbelo, plobsing:

Do we still need the auto_libjit branch? Or is it safe for me to remove it?

kid51

  Changed 5 years ago by jkeenan

Per plobsing on  #parrotsketch today:

* new branch, libjit_framebuilder supercedes auto_libjit branch

So I'm deleting that branch from our repository.

This ticket, however, will remain open until the libjit_framebuilder branch is merged.

kid51

  Changed 5 years ago by plobsing

  • status changed from assigned to closed
  • resolution set to wontfix

I have packaged this functionality into a separate library at  http://github.com/plobsing/parrot-libjit-fb .

This ticket is no longer necessary.

Note: See TracTickets for help on using tickets.