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.

Sign up for GitHub

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

Refactor error handling code to eliminate internal ID calls #4453

Merged
merged 10 commits into from
May 9, 2024
Merged

Refactor error handling code to eliminate internal ID calls #4453

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
36eb05f
Refactor error handling code to eliminate internal ID calls.
qkoziol May 1, 2024
06118ad
Committing clang-format changes
github-actions[bot] May 1, 2024
a5dcea4
Remove generated H5E headers
qkoziol May 2, 2024
4eaadb7
Merge remote-tracking branch 'origin/develop' into h5e_refactor
qkoziol May 3, 2024
6057db9
Merge remote-tracking branch 'origin/develop' into h5e_refactor
qkoziol May 7, 2024
ebd8051
Add new generated error headers to .gitignore
qkoziol May 7, 2024
2d380a3
Update H5E__copy_stack_entry() to reflect its actual requirements
qkoziol May 7, 2024
a0bd026
Committing clang-format changes
github-actions[bot] May 7, 2024
1521c90
Correct typo
qkoziol May 8, 2024
752523a
Remove unused variable
qkoziol May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ m4/ltversion.m4
m4/lt~obsolete.m4
src/H5Edefin.h
src/H5Einit.h
src/H5E.h
qkoziol marked this conversation as resolved.
Show resolved Hide resolved
src/H5Emajdef.h
src/H5Emindef.h
src/H5Epubgen.h
src/H5Eterm.h
src/H5config.h.in
Expand Down
2 changes: 2 additions & 0 deletions bin/h5vers
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ sub setvers {
$_[0] =~ s/^(\#\s*define\s+H5_VERS_MINOR\s+)\d+/$1$vers[1]/m;
$_[0] =~ s/^(\#\s*define\s+H5_VERS_RELEASE\s+)\d+/$1$vers[2]/m;
$_[0] =~ s/^(\#\s*define\s+H5_VERS_SUBRELEASE\s+\")[^\"]*/$1$vers[3]/m;
$_[0] =~ s/^(\#\s*define\s+H5_VERS_STR\s+\")[^\"]*/
sprintf("%s%d.%d.%d%s%s", $1, @vers[0,1,2], $vers[3]?"-":"", $vers[3])/me;
$_[0] =~ s/^(\#\s*define\s+H5_VERS_INFO\s+\")[^\"]*/
sprintf("%sHDF5 library version: %d.%d.%d%s%s", $1, @vers[0,1,2],
$vers[3]?"-":"", $vers[3])/me;
Expand Down
177 changes: 136 additions & 41 deletions bin/make_err
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ sub print_warning ($) {
my $fh = shift;

print $fh "\n/* Generated automatically by bin/make_err -- do not edit */\n";
print $fh "/* Add new errors to H5err.txt file */\n\n";
print $fh "/* Add new errors to H5err.txt file */\n";
}

##############################################################################
Expand Down Expand Up @@ -92,7 +92,7 @@ sub parse_line ($) {
# Get the major error's name & description
($name, $desc) = ($line =~ /^\s*MAJOR,\s*(\w*),\s*(.*)\n/);
#print "MAJOR: name=$name, desc=$desc\n";

# Check if the name already exists as a major or minor error message
if(exists($major{$name}) || exists($minor{$name})) {
die "duplicated name: $name";
Expand All @@ -103,12 +103,12 @@ sub parse_line ($) {
}
# Parse minor error lines
elsif($line =~ /^\s*MINOR,/) {
my $min_section; # Minor errors have a section they below to also
my $min_section; # Minor errors have a section they belong to also

# Get the minor error's section, name & description
($min_section, $name, $desc) = ($line =~ /^\s*MINOR,\s*(\w*),\s*(\w*),\s*(.*)\n/);
#print "MINOR: min_section=$min_section, name=$name, desc=$desc\n";

# Check for valid section
if(!exists($section{$min_section})) {
die "unknown section: $min_section";
Expand All @@ -121,7 +121,7 @@ sub parse_line ($) {

# Store the minor errors in a hash table, indexed by the name
$minor{$name}=$desc;

# Add the minor error to the list for the section
push @{$section_list{$min_section}}, $name;
}
Expand All @@ -130,7 +130,7 @@ sub parse_line ($) {
# Get the section's name & description
($name, $desc) = ($line =~ /^\s*SECTION,\s*(\w*),\s*(.*)\n/);
#print "SECTION: name=$name, desc=$desc\n";

# Check if the section has already been defined
if(exists($section{$name})) {
die "duplicated name: $name";
Expand Down Expand Up @@ -178,25 +178,25 @@ sub create_public ($) {
print HEADER "\n/*********************/\n";
print HEADER "/* Major error codes */\n";
print HEADER "/*********************/\n\n";
foreach $name (keys %major) {
foreach $name (sort keys %major) {
printf HEADER "#define %-20s (H5OPEN %s_g)\n",$name,$name;
}
foreach $name (keys %major) {
foreach $name (sort keys %major) {
printf HEADER "H5_DLLVAR hid_t %-20s /* %s */\n","${name}_g;",$major{$name};
}

# Iterate over all the minor error sections
print HEADER "\n/*********************/\n";
print HEADER "/* Minor error codes */\n";
print HEADER "/*********************/\n";
while ( ($sect_name, $sect_desc) = each (%section)) {
print HEADER "\n/* $sect_desc */\n";
foreach $sect_name (sort keys %section) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort the errors alphabetically by name, so they have stable lists in the generated headers.

print HEADER "\n/* $section{$sect_name} */\n";

# Iterate over all the minor errors in each section
for $name ( @{$section_list{$sect_name}}) {
for $name (sort @{$section_list{$sect_name}}) {
printf HEADER "#define %-20s (H5OPEN %s_g)\n",$name,$name;
}
for $name ( @{$section_list{$sect_name}}) {
for $name (sort @{$section_list{$sect_name}}) {
printf HEADER "H5_DLLVAR hid_t %-20s /* %s */\n","${name}_g;",$minor{$name};
}
}
Expand All @@ -213,18 +213,98 @@ sub create_public ($) {
close HEADER;
}

##############################################################################
# Create the generated portion of the H5E major message definition code
#
sub create_majdef ($) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates a static const message for each library-internal major error message, so they aren't malloc'ed/free'd at init/term.

my $prefix = shift; # Get the prefix for the generated file
my $file = "H5Emajdef.h"; # Name of file to generate
my $name; # Name of error message

# Rename previous file
# rename "${prefix}${file}", "${prefix}${file}~" or die "unable to make backup";

# Open new header file
open HEADER, ">${prefix}${file}" or die "unable to modify source";

# Create file contents

print_copyright(*HEADER);
print_warning(*HEADER);
print_startprotect(*HEADER, $file);

# Iterate over all the major errors
print HEADER "\n/***********************************/\n";
print HEADER "/* Major error message definitions */\n";
print HEADER "/***********************************/\n\n";
print HEADER "/* clang-format off */\n";
foreach $name (sort keys %major) {
printf HEADER "static const H5E_msg_t ${name}_msg_s = {false, \"${major{$name}}\", H5E_MAJOR, &H5E_err_cls_s};\n";
}
print HEADER "/* clang-format on */\n";

print_endprotect(*HEADER, $file);

# Close header file
close HEADER;
}

##############################################################################
# Create the generated portion of the H5E minor message definition code
#
sub create_mindef ($) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates a static const message for each library-internal minor error message, so they aren't malloc'ed/free'd at init/term.

my $prefix = shift; # Get the prefix for the generated file
my $file = "H5Emindef.h"; # Name of file to generate
my $name; # Name of error message
my $sect_name; # Section of minor error messages

# Rename previous file
# rename "${prefix}${file}", "${prefix}${file}~" or die "unable to make backup";

# Open new header file
open HEADER, ">${prefix}${file}" or die "unable to modify source";

# Create file contents

print_copyright(*HEADER);
print_warning(*HEADER);
print_startprotect(*HEADER, $file);

# Iterate over all the minor error sections
print HEADER "\n/***********************************/\n";
print HEADER "/* Minor error message definitions */\n";
print HEADER "/***********************************/\n\n";
print HEADER "/* clang-format off */\n";
foreach $sect_name (sort keys %section) {
print HEADER "\n/* $sect_name: $section{$sect_name} */\n";

# Iterate over all the minor errors in each section
for $name (sort @{$section_list{$sect_name}}) {
printf HEADER "static const H5E_msg_t ${name}_msg_s = {false, \"${minor{$name}}\", H5E_MINOR, &H5E_err_cls_s};\n";
}
}
print HEADER "/* clang-format on */\n";

print_endprotect(*HEADER, $file);

# Close header file
close HEADER;
}

##############################################################################
# Create the generated portion of the H5E initialization code
#
sub create_init ($) {
my $prefix = shift; # Get the prefix for the generated file
my $file = "H5Einit.h"; # Name of file to generate
my $name; # Name of error message
my $last_name; # Name of previous error message
my $desc; # Description of error message
my $sect_name; # Section of minor error messages
my $sect_desc; # Description of section
my $first_major = 0; # Whether the first major error code was saved
my $first_minor = 0; # Whether the first minor error code was saved
my $num_msg; # Number of messages
qkoziol marked this conversation as resolved.
Show resolved Hide resolved

# Rename previous file
# rename "${prefix}${file}", "${prefix}${file}~" or die "unable to make backup";
Expand All @@ -239,14 +319,14 @@ sub create_init ($) {
print_startprotect(*HEADER, $file);

# Iterate over all the major errors
$num_msg = 0;
print HEADER "\n/*********************/\n";
print HEADER "/* Major error codes */\n";
print HEADER "/*********************/\n\n";
foreach $name (keys %major) {
print HEADER " "x(0*$indent),"assert(${name}_g==H5I_INVALID_HID);\n";
print HEADER " "x(0*$indent),"if((msg = H5E__create_msg(cls, H5E_MAJOR, \"${major{$name}}\"))==NULL)\n";
print HEADER " "x(1*$indent),"HGOTO_ERROR(H5E_ERROR, H5E_CANTINIT, FAIL, \"error message initialization failed\");\n";
print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, msg, false))<0)\n";
foreach $name (sort keys %major) {
print HEADER "/* $name */\n";
print HEADER " "x(0*$indent),"assert(H5I_INVALID_HID == ${name}_g);\n";
print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, &${name}_msg_s, false)) < 0)\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set up the major error IDs, to use the static variables.

print HEADER " "x(1*$indent),"HGOTO_ERROR(H5E_ERROR, H5E_CANTREGISTER, FAIL, \"can't register error message\");\n";
if ($first_major == 0) {
print HEADER " "x(0*$indent),"\n/* Remember first major error code ID */\n";
Expand All @@ -255,24 +335,25 @@ sub create_init ($) {
$first_major = 1;
}
$last_name = $name;
$num_msg++;
}
print HEADER " "x(0*$indent),"\n/* Remember last major error code ID */\n";
print HEADER " "x(0*$indent),"assert(H5E_last_maj_id_g==H5I_INVALID_HID);\n";
print HEADER " "x(0*$indent),"H5E_last_maj_id_g = ${last_name}_g;\n\n";

# Iterate over all the minor error sections
$num_msg = 0;
print HEADER "\n/*********************/\n";
print HEADER "/* Minor error codes */\n";
print HEADER "/*********************/\n\n";
while ( ($sect_name, $sect_desc) = each (%section)) {
print HEADER "\n"," "x(0*$indent),"/* $sect_desc */\n";
foreach $sect_name (sort keys %section) {
print HEADER "\n/* $section{$sect_name} */\n";

# Iterate over all the minor errors in each section
for $name ( @{$section_list{$sect_name}}) {
print HEADER " "x(0*$indent),"assert(${name}_g==H5I_INVALID_HID);\n";
print HEADER " "x(0*$indent),"if((msg = H5E__create_msg(cls, H5E_MINOR, \"${minor{$name}}\"))==NULL)\n";
print HEADER " "x(1*$indent),"HGOTO_ERROR(H5E_ERROR, H5E_CANTINIT, FAIL, \"error message initialization failed\");\n";
print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, msg, true))<0)\n";
for $name (sort @{$section_list{$sect_name}}) {
print HEADER "/* $name */\n";
print HEADER " "x(0*$indent),"assert(H5I_INVALID_HID == ${name}_g);\n";
print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, &${name}_msg_s, false)) < 0)\n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set up the minor error IDs, to use the static variables.

print HEADER " "x(1*$indent),"HGOTO_ERROR(H5E_ERROR, H5E_CANTREGISTER, FAIL, \"can't register error message\");\n";

if ($first_minor == 0) {
Expand All @@ -282,6 +363,7 @@ sub create_init ($) {
$first_minor = 1;
}
$last_name = $name;
$num_msg++;
}
}
print HEADER " "x(0*$indent),"\n/* Remember last minor error code ID */\n";
Expand Down Expand Up @@ -319,25 +401,25 @@ sub create_term ($) {

# Iterate over all the major errors
print HEADER "\n/* Reset major error IDs */\n";
foreach $name (keys %major) {
print HEADER " "x($indent),"\n${name}_g=";
foreach $name (sort keys %major) {
print HEADER " "x($indent),"${name}_g =\n";
}
print HEADER " H5I_INVALID_HID;\n";
print HEADER " "x(0*$indent),"H5E_first_maj_id_g = H5I_INVALID_HID;\n\n";
print HEADER " "x(2*$indent),"H5I_INVALID_HID;\n";
print HEADER "\n"," "x(0*$indent),"H5E_first_maj_id_g = H5I_INVALID_HID;\n";
print HEADER " "x(0*$indent),"H5E_last_maj_id_g = H5I_INVALID_HID;\n\n";

# Iterate over all the minor error sections
print HEADER "\n/* Reset minor error IDs */\n";
while ( ($sect_name, $sect_desc) = each (%section)) {
print HEADER "\n"," "x(0*$indent),"\n/* $sect_desc */";
foreach $sect_name (sort keys %section) {
print HEADER "\n/* $sect_name: $section{$sect_name} */\n";

# Iterate over all the minor errors in each section
for $name ( @{$section_list{$sect_name}}) {
print HEADER " "x($indent),"\n${name}_g=";
for $name (sort @{$section_list{$sect_name}}) {
print HEADER " "x($indent),"${name}_g =\n";
}
}
print HEADER " H5I_INVALID_HID;\n";
print HEADER " "x(0*$indent),"H5E_first_min_id_g = H5I_INVALID_HID;\n\n";
print HEADER " "x(2*$indent),"H5I_INVALID_HID;\n";
print HEADER "\n"," "x(0*$indent),"H5E_first_min_id_g = H5I_INVALID_HID;\n";
print HEADER " "x(0*$indent),"H5E_last_min_id_g = H5I_INVALID_HID;\n\n";

print_endprotect(*HEADER, $file);
Expand All @@ -356,6 +438,7 @@ sub create_define ($) {
my $desc; # Description of error message
my $sect_name; # Section of minor error messages
my $sect_desc; # Description of section
my $num_msg; # Number of messages

# Rename previous file
# rename "${prefix}${file}", "${prefix}${file}~" or die "unable to make backup";
Expand All @@ -370,21 +453,29 @@ sub create_define ($) {
print_startprotect(*HEADER, $file);

# Iterate over all the major errors
$num_msg = 0;
print HEADER "\n/* Major error IDs */\n";
foreach $name (keys %major) {
printf HEADER "hid_t %-20s = FAIL; /* %s */\n","${name}_g",$major{$name};
foreach $name (sort keys %major) {
printf HEADER "hid_t %-20s = H5I_INVALID_HID; /* %s */\n","${name}_g",$major{$name};
$num_msg++;
}
print HEADER "\n/* Number of major error messages */\n";
printf HEADER "#define H5E_NUM_MAJ_ERRORS %d\n", $num_msg;

# Iterate over all the minor error sections
$num_msg = 0;
print HEADER "\n/* Minor error IDs */\n";
while ( ($sect_name, $sect_desc) = each (%section)) {
print HEADER "\n/* $sect_desc */\n";
foreach $sect_name (sort keys %section) {
print HEADER "\n/* $sect_name: $section{$sect_name} */\n";

# Iterate over all the minor errors in each section
for $name ( @{$section_list{$sect_name}}) {
printf HEADER "hid_t %-20s = FAIL; /* %s */\n","${name}_g",$minor{$name};
for $name (sort @{$section_list{$sect_name}}) {
printf HEADER "hid_t %-20s = H5I_INVALID_HID; /* %s */\n","${name}_g",$minor{$name};
$num_msg++;
}
}
print HEADER "\n/* Number of minor error messages */\n";
printf HEADER "#define H5E_NUM_MIN_ERRORS %d\n", $num_msg;

print_endprotect(*HEADER, $file);

Expand All @@ -409,10 +500,14 @@ for $file (@ARGV) {
}
}
close SOURCE;

# Create header files
print "Generating 'H5Epubgen.h'\n";
create_public($prefix);
print "Generating 'H5Emajdef.h'\n";
create_majdef($prefix);
print "Generating 'H5Emindef.h'\n";
create_mindef($prefix);
print "Generating 'H5Einit.h'\n";
create_init($prefix);
print "Generating 'H5Eterm.h'\n";
Expand Down
2 changes: 2 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,8 @@ set (H5_PRIVATE_HEADERS

${HDF5_SRC_DIR}/H5Edefin.h
${HDF5_SRC_DIR}/H5Einit.h
${HDF5_SRC_DIR}/H5Emajdef.h
${HDF5_SRC_DIR}/H5Emindef.h
${HDF5_SRC_DIR}/H5Epkg.h
${HDF5_SRC_DIR}/H5Eprivate.h
${HDF5_SRC_DIR}/H5Eterm.h
Expand Down
4 changes: 2 additions & 2 deletions src/H5D.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2137,7 +2137,7 @@ H5Dformat_convert(hid_t dset_id)

/* Convert the dataset */
if (H5VL_dataset_optional(vol_obj, &vol_cb_args, H5P_DATASET_XFER_DEFAULT, H5_REQUEST_NULL) < 0)
HGOTO_ERROR(H5E_DATASET, H5E_INTERNAL, FAIL, "can't convert dataset format");
HGOTO_ERROR(H5E_DATASET, H5E_CANTUPDATE, FAIL, "can't convert dataset format");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use minor error code instead of major.


done:
FUNC_LEAVE_API(ret_value)
Expand Down Expand Up @@ -2442,7 +2442,7 @@ H5Dchunk_iter(hid_t dset_id, hid_t dxpl_id, H5D_chunk_iter_op_t op, void *op_dat

/* Iterate over the chunks */
if ((ret_value = H5VL_dataset_optional(vol_obj, &vol_cb_args, dxpl_id, H5_REQUEST_NULL)) < 0)
HERROR(H5E_BADITER, H5E_BADITER, "error iterating over dataset chunks");
HERROR(H5E_DATASET, H5E_BADITER, "error iterating over dataset chunks");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use major error code instead of minor.


done:
FUNC_LEAVE_API(ret_value)
Expand Down
2 changes: 1 addition & 1 deletion src/H5Dint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,7 @@ H5D_open(const H5G_loc_t *loc, hid_t dapl_id)
/* Check if dataset was already open */
if (NULL == (shared_fo = (H5D_shared_t *)H5FO_opened(dataset->oloc.file, dataset->oloc.addr))) {
/* Clear any errors from H5FO_opened() */
H5E_clear_stack(NULL);
H5E_clear_stack();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internal to the library, we always clear the default error stack, so update H5E_clear_stack() to do that.


/* Open the dataset object */
if (H5D__open_oid(dataset, dapl_id) < 0)
Expand Down
Loading