Debug improvements #2602

ChrisJefferson · 2018-07-02T20:42:23Z

This PR:

a) Fixes MEMORY_CANARY and adds a configure option for it.

b) Adds some awful, but at least existing, documentation for kernel debugging (further improvements / extensions welcome).

codecov · 2018-07-02T21:37:52Z

Codecov Report

Merging #2602 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2602      +/-   ##
==========================================
+ Coverage    74.8%    74.8%   +<.01%     
==========================================
  Files         479      479              
  Lines      242243   242243              
==========================================
+ Hits       181198   181200       +2     
+ Misses      61045    61043       -2

Impacted Files	Coverage Δ
src/gasman.c	`87.84% <ø> (ø)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`64.7% <0%> (+0.51%)`	⬆️

olexandr-konovalov · 2018-07-02T21:54:29Z

doc/dev/lib.xml

+<Section Label="Sect-debugging">
+<Heading>Debugging the GAP Kernel</Heading>
+
+The GAP kernel supports a variety of options which enabled when running <C>configure</C>,


GAP should be &GAP; throughout because as an entity it is displayed in a different font (as a <Package element).

Required: This sentence seems to be missing a word, like "which can be enabled by running" ? Or maybe reorder the sentence for readability (and use the appropriate XML for filenames):

The GAP kernel supports a variety of compile-time options to help writing and debugging GAP kernel code.
They can be enabled by passing one or more of the following options to the configure script.

olexandr-konovalov · 2018-07-02T21:55:55Z

doc/dev/lib.xml

+the Configuration line when GAP is started.
+<P/>
+
+Known bugs: GAP will crash if IO_fork is called while memory checking is enabled.


IO_fork should be a cross-reference to the IO package manual.

fingolfin

Two changes in the last commit really should be made, the rest is optional.

src/gasman.c

+#define CANARY_ENABLE_VALGRIND() VALGRIND_ENABLE_ERROR_REPORTING
+
+void CHANGED_BAG(Bag bag) {
+    CANARY_DISABLE_VALGRIND();


fingolfin · 2018-07-03T07:02:00Z

configure.ac

+    [AC_DEFINE([MEMORY_CANARY], [1], [define if build with valgrind extensions])],
+    [enable_valgrind=no]
+    )
+AC_MSG_CHECKING([whether to add valgrind extensions to GASMAN])


Optional: Instead of "add" here and above, I'd use "enable", to match the name of the option. Also, to me "add" has the wrong meaning here, but that's probably just me being a non-native speaker.

fingolfin · 2018-07-03T07:03:02Z

configure.ac

@@ -246,6 +246,18 @@ AC_ARG_ENABLE([memory-checking],
 AC_MSG_CHECKING([whether to enable memory checking])
 AC_MSG_RESULT([$enable_memory_checking])

+AC_ARG_ENABLE([valgrind],
+    [AS_HELP_STRING([--enable-valgrind], [add valgrind extensions to GASMAN])],
+    [AC_DEFINE([MEMORY_CANARY], [1], [define if build with valgrind extensions])],


optional: build -> built? Though everywhere else, we actually use "building"

configure.ac

+AS_IF([test "x$enable_valgrind" != "xno" -a "x$enable_memory_checking" != "xno"],
+    AC_MSG_ERROR([--enable-valgrind and --enable-memory-checking cannot be used at the same time])
+    )
+


fingolfin · 2018-07-03T07:07:40Z

doc/dev/lib.xml

+<Section Label="Sect-debugging">
+<Heading>Debugging the GAP Kernel</Heading>
+
+The GAP kernel supports a variety of options which enabled when running <C>configure</C>,


Required: This sentence seems to be missing a word, like "which can be enabled by running" ? Or maybe reorder the sentence for readability (and use the appropriate XML for filenames):

The GAP kernel supports a variety of compile-time options to help writing and debugging GAP kernel code.
They can be enabled by passing one or more of the following options to the configure script.

fingolfin · 2018-07-03T07:09:24Z

doc/dev/lib.xml

+<P/>
+
+After configuring, the memory checking must still be turned on. This can be done
+either by passing <C>--</C> to GAP's command line, or calling <C>GASMAN_MEM_CHECK(1)</C>.


Required: Shouldn't that -- be --enableMemCheck?

fingolfin · 2018-07-03T07:11:01Z

doc/dev/lib.xml

+assertions are implemented using the <C>GAP_ASSERT</C> macro. These
+assertions are disabled when GAP is compiled without <C>--enable-debug</C>.
+When <C>--enable-debug</C> is enabled, <C>KernelDebug</C> will be shown in
+the Configuration line when GAP is started.


Optional: Why is Configuration in upper case? If it is meant to "quote" the output of GAP, I'd mark it as such, e.g.

the line starting with Configuration: when &GAP; is started.
Else, I'd use lower case. Same below.

fingolfin · 2018-07-03T07:12:49Z

doc/dev/lib.xml

+corruption relating to GASMAN (GAP's memory manager). They cannot both be enabled
+at the same time.
+<P/>
+<C>--enable-memory-checking</C> makes GAP check for C pointers to the content of Bags


I'd not capitalize "Bag" here, though one might want to insert an explanation of what a bag is. Then again, it makes little sense to use this option if one does not know what a GAP "bag" is. Perhaps Insert in the previous line something like this:

The following explanations assume you are somewhat familiar with the internals of GASMAN (see e.g. src/gasman.c for more information).

fingolfin · 2018-07-03T07:13:26Z

doc/dev/lib.xml

+
+At present, this <C>--enable-valgrind</C> only checks for invalid writes to the
+last bag which was created. Also, this option does not do anything unless GAP
+is run through Valgrind (for example by running <C>valgrind ./gap</C>.


Why is Valgrind capitalized here but not above?

fingolfin · 2018-07-03T07:15:46Z

doc/dev/lib.xml

+<P/>
+
+<C>--enable-valgrind</C> makes changes to GASMAN so it is compatible with the
+valgrind memory checking program. Without this, valgrind will report many 


Optional: How about adding a URL here?

<URL Text="valgrind">http://valgrind.org</URL> memory checking program. Without this [...]

ChrisJefferson · 2018-07-03T12:08:39Z

All improvements seem good, so I have made all of them. The documentation should still be significantly improved, but at least something exists now.

The only change I didn't make was cross-linking to IO_fork because for some reason I was getting errors I didn't understand, and making to the dev manual is quite annoying as it takes so long to run make doc.

fingolfin

This could be merged now, though I still left two optional suggestions

fingolfin · 2018-07-03T12:28:21Z

doc/dev/lib.xml

+at the same time.
+
+These options assume you are familiar somewhat familiar with the internals of GASMAN
+(see gasman.c for more information). In particular, GASMAN represents memory using


strictly speaking, this should be <F>gasman.c</F>. But can also be changed later.

fingolfin · 2018-07-03T12:29:22Z

doc/dev/lib.xml

+the line starting <C>Configuration:</C> when &GAP; is started.
+<P/>
+
+Known bugs: &GAP; will crash if IO_fork is called while memory checking is enabled.


If this is not a ref for technical reasons, then shouldn't it at least be <C>IO_fork</C>? And perhaps add "from the IO package"?

fingolfin · 2018-09-13T07:51:55Z

As these are low-level kernel changes, I don't think this needs to be documented in the release notes... But I am not 100% certain (it depends on how exactly we define what should and what shouldn't go into release notes.... and at whom we target them: end-users only? Or also kernel devs? Thoughts?

olexandr-konovalov · 2018-09-13T09:35:58Z

Since this adds a new section "Debugging the GAP Kernel" to the manual, I would add to release notes some text conveniently linking it to that section.

olexandr-konovalov reviewed Jul 2, 2018

View reviewed changes

fingolfin requested changes Jul 3, 2018

View reviewed changes

Move MEMORY_CANARY code so it is after definition of ChangedBags

ce7671f

ChrisJefferson force-pushed the debug-doc branch from 9049245 to 7481bb0 Compare July 3, 2018 11:43

Add configure script option for memory canary

7447c7e

ChrisJefferson force-pushed the debug-doc branch 3 times, most recently from 284b4d4 to 5062251 Compare July 3, 2018 12:07

fingolfin approved these changes Jul 3, 2018

View reviewed changes

ChrisJefferson force-pushed the debug-doc branch from 5062251 to 9fe22dc Compare July 3, 2018 14:07

Add some minimal documentation for kernel debugging

9b3e6ff

ChrisJefferson force-pushed the debug-doc branch from 9fe22dc to 9b3e6ff Compare July 3, 2018 14:07

fingolfin merged commit 7da3eb1 into gap-system:master Jul 4, 2018

ChrisJefferson deleted the debug-doc branch August 20, 2018 10:06

fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 13, 2018

fingolfin removed the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Sep 13, 2018

olexandr-konovalov added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Sep 13, 2018

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug improvements #2602

Debug improvements #2602

ChrisJefferson commented Jul 2, 2018

codecov bot commented Jul 2, 2018 •

edited

Loading

olexandr-konovalov Jul 2, 2018

fingolfin Jul 3, 2018

olexandr-konovalov Jul 2, 2018

fingolfin left a comment

This comment was marked as resolved.

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

This comment was marked as resolved.

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

ChrisJefferson commented Jul 3, 2018

fingolfin left a comment

fingolfin Jul 3, 2018

fingolfin Jul 3, 2018

fingolfin commented Sep 13, 2018

olexandr-konovalov commented Sep 13, 2018

Debug improvements #2602

Debug improvements #2602

Conversation

ChrisJefferson commented Jul 2, 2018

codecov bot commented Jul 2, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisJefferson commented Jul 3, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Sep 13, 2018

olexandr-konovalov commented Sep 13, 2018

codecov bot commented Jul 2, 2018 •

edited

Loading