Re: What about MIPS support?

[Josh Stone <jistone@redhat.com>]

On 10/15/2012 02:21 PM, Victor Kamensky wrote:
> Hi All,
> 
> It took us much much longer than originally expected. But it is better 
> late than never. Here is our patches tarbal attached. Inside of tarbal 
> (README file) and copied below I put short description of each patch. Some 
> of patches are Cisco specific and may not be any interest for you. Some 
> like MIPS support, cross compilation improvements, misc general fixes (i.e 
> systemtap issue 6977), I think, could be quite useful to community. Since 
> it is quilt patch series and patches may have patch apply order issues, we 
> are publishing all of them, exactly as we use/apply them, regardless 
> whether we consider them Cisco specific or not.
> 
> As it was discussed, for now, I am just dropping our patches as is. Latter 
> as time permits, I can work on cleaning them up, moving to latest 
> systemtap git and try to prepare them for commit into systemtap tree. It 
> would be great if folks could take a look at patches and/or patches 
> description and indicate priorities/interests for specific patches on 
> which I could start working first.

Thanks!

For posterity and hopefully easier review, I applied this series as git
commits, and pushed them out for anyone else to see too:
http://sourceware.org/git/gitweb.cgi?p=systemtap.git;a=shortlog;h=refs/heads/systemtap-1.6-cisco-patches

Since I like to preserve such details, I set you as the --author (except
Maneesh on mips-uprobes), --date from the files in the tarball, and
commit messages from your README descriptions.

> Please advise if you would like me to post the same on different subject 
> line on this mailing list (since it covers more than just mips support). I 
> thought I just post it on original thread to get it proper closure.

This thread is fine with me.  When you start porting patches forward, if
you want to start new threads, that's fine too.

It may be a while before I can review these patches fully, but I do have
a few comments as I glance over them now.  Any patches I skip over just
means I don't have any comment, not total disinterest. ;)

> systemtap-unwind-table-size.patch - IOS code is too big, we are hitting limit
>      on unwind symbol table size. Increase the limit for now. In future it would
>      be good code to adjust those values dynamically.

You went from 6M to 48M - does IOS really need that much?  It's an
arbitrary limit, but we should try to keep it low if possible.

> systemtap-userland_caller.patch - Trying to introduce ucaller_addr and ucaller
>      systemtap function. Most likely this code does not work. But currently
>      patch is in our series file so keeping it for now.

I believe the new ustack() along with usymname() can handle this too.

> systemtap-biendian.patch - Support ICC biendian feature. It is described at
>      http://software.intel.com/en-us/articles/dwarf-extensions-for-bi-endian-support/. Most
>      like this code has no interest to anyone except Cisco.

Well FWIW, I wouldn't be opposed to such ICC-specific patches, as long
as it doesn't break the primary GCC support.

> systemtap-old_compiler-task_finder.patch - Older gcc compiler (like gcc 3.4.x)
>      produce compilation warning here, incorrectly assuming that dentry may be
>      used uninitialized

Wade Farnsworth also fixed this, commit 090a235b.

> systemtap-cross_compile_helper.patch - Cross compilation assist. Actually
>      include a lot of different small things. Here is the list of them (may not
>      be complete):

For the sysroot parts, Wade also added a --sysroot option, so your
changes should be reconciled with that.  And if possible, please break
this one down into discrete pieces.

> systemtap-data-in_another_cu.patch - Fix for issue
>      6977 http://sourceware.org/bugzilla/show_bug.cgi?id=6977. We found it is
>      very annoying and limiting for script writers, so we fixed it. It deals
>      with  situation of accessing global variables that are defined in *other*
>      compilation units.

If this is generally improved, that's great!  Mark also added a @var
accessor, which is a similar idea, but maybe not quite the same.

> systemtap-line_range_issue_fix.patch - memory leak and crash in
>      iterate_over_srcfile_lines in case of line ranges.

If crashes like this are still present in master git, then let's
definitely prioritize them!

> systemtap-kernel_source_tree.patch - Option -T allows to specify location of
>      kernel sources. Normally needed only in cross compilation environment.

The existing -r can take either a DIR or a RELEASE - is that not enough?

> systemtap-remote_hack.patch - allows remote_uris mechanism to work in cross
>      compilation environment, where target kernel release won't be the same as
>      host kernel release

Yeah, this was a bad oversight, fixed in commit 19da0351.

> systemtap-cross_testsuite.patch - Attempt to change testsuite to operate in
>      cross compilation environment where access to target happens either through
>      dejagnu remote target facilities or through stap --remote mechanism.
>      Massive patch, across bunch of test cases, not sure whether community has
>      any interest in it. It was used by us to validate systemtap operation on our
>      targets. In order to run, it requires stap board specific config (not
>      provided here), which would specify things like stap_board_args, and others.

I definitely think testing is great for cross-compilation, and it's also
nice for it to exercise --remote as that has little in the testsuite either.

David's been doing a similar big change to support the different
runtimes (for the dyninst feature), which I think covers a lot of the
same areas in adding additional parameters to various test runs.
Hopefully that overlap is actually helpful here.

> systemtap-enable_vma_tracker.patch - cover corner cases where
>      _stp_umodule_relocate is used but vma_tracker is not enabled, while it
>      should be, becuase it uses stap_find_vma_map_info_user function.

If this is still lacking, we should definitely fix it.

Finally, I noticed systemtap-mv_preempt_rt.patch in your tarball, which
isn't the series or README.  That one looks more hacky than the rest, so
maybe it should be left out. :)  But if there's a real purpose behind
that patch, let's hear it.


Josh