Fix #9331 - Crash if window construction is zero due to bad construction name #9646

jmarrec · 2022-09-06T15:30:54Z

Pull request overview

Fixes Crash if window construction is zero due to bad construction name #9331

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

```shell [ RUN ] EnergyPlusFixture.Wrong_Window_Construction energyplus_tests: /home/julien/Software/Others/EnergyPlus/third_party/ObjexxFCL/src/ObjexxFCL/Array1.hh:861: T& ObjexxFCL::Array1< <template-parameter-1-1> >::operator()(int) [with T = EnergyPlus::Construction::ConstructionProps]: Assertion `contains( i )' failed. ``` backtrace: ```shell frame #5: 0x00005555632f28ca energyplus_tests`EnergyPlus::SurfaceGeometry::CheckWindowShadingControlFrameDivider(state=0x000055556dc601b0, cRoutineName=(_M_len = 19, _M_str = "GetHTSubSurfaceData"), ErrorsFound=0x00007fffffffb8e6, SurfNum=2, FrameField=6) at SurfaceGeometry.cc:5972:106 5969 } // End of check if window has a construction 5970 } 5971 -> 5972 if (state.dataConstruction->Construct(state.dataSurfaceGeometry->SurfaceTmp(SurfNum).Construction).WindowTypeEQL) { 5973 if (state.dataSurfaceGeometry->SurfaceTmp(SurfNum).FrameDivider > 0) { 5974 // Equivalent Layer window does not have frame/divider model 5975 ShowSevereError(state ```

…deemed invalid, skip to the next

rraustad · 2022-09-10T23:55:04Z

src/EnergyPlus/SurfaceGeometry.cc

                ErrorsFound = true;
+                continue;


I don't have a problem with using a continue when "very" severe errors are found. But I wonder if there are some errors that won't show up until the user fixes the first one found. It may not be necessary to continue on some of these warnings so that as many warnings could be reported the first time through. Which one of these input errors actually causes the crash?

I understand the sentiment here. If we could issue all 3 errors up front on the subsurface, it would be good to let the user know all 3 errors right away. Otherwise they will end up possibly running 3 iterations to solve all the issues. But I also understand the idea of wanting to avoid issues from subsequent checking. I could be swayed either way. Perhaps some extra description of the exact errors we are bypassing by each continue would help.

I am perfectly fine if we revert dcbda05, the 59f5c65 is enough to avoid the crash

Side note: I actually have a stashed commit that went even further and wrapped every GetXXX call inside an if (!ErrorsFound) so it'll bail early here:

EnergyPlus/src/EnergyPlus/SurfaceGeometry.cc

Lines 1284 to 1347 in 0c3de81

GetDetShdSurfaceData(state, ErrorsFound, NumSurfs, TotDetachedFixed, TotDetachedBldg);

GetRectDetShdSurfaceData(state, ErrorsFound, NumSurfs, TotRectDetachedFixed, TotRectDetachedBldg);

GetHTSurfaceData(state,

ErrorsFound,

NumSurfs,

TotHTSurfs,

TotDetailedWalls,

TotDetailedRoofs,

TotDetailedFloors,

state.dataSurfaceGeometry->BaseSurfCls,

state.dataSurfaceGeometry->BaseSurfIDs,

NeedToAddSurfaces);

GetRectSurfaces(state,

ErrorsFound,

NumSurfs,

TotRectExtWalls,

TotRectIntWalls,

TotRectIZWalls,

TotRectUGWalls,

TotRectRoofs,

TotRectCeilings,

TotRectIZCeilings,

TotRectGCFloors,

TotRectIntFloors,

TotRectIZFloors,

state.dataSurfaceGeometry->BaseSurfIDs,

NeedToAddSurfaces);

GetHTSubSurfaceData(state,

ErrorsFound,

NumSurfs,

TotHTSubs,

state.dataSurfaceGeometry->SubSurfCls,

state.dataSurfaceGeometry->SubSurfIDs,

AddedSubSurfaces,

NeedToAddSubSurfaces);

GetRectSubSurfaces(state,

ErrorsFound,

NumSurfs,

TotRectWindows,

TotRectDoors,

TotRectGlazedDoors,

TotRectIZWindows,

TotRectIZDoors,

TotRectIZGlazedDoors,

state.dataSurfaceGeometry->SubSurfIDs,

AddedSubSurfaces,

NeedToAddSubSurfaces);

GetAttShdSurfaceData(state, ErrorsFound, NumSurfs, TotShdSubs);

GetSimpleShdSurfaceData(state, ErrorsFound, NumSurfs, TotOverhangs, TotOverhangsProjection, TotFins, TotFinsProjection);

GetIntMassSurfaceData(state, ErrorsFound, NumSurfs);

state.dataSurface->TotSurfaces = NumSurfs + AddedSubSurfaces + NeedToAddSurfaces + NeedToAddSubSurfaces;

if (ErrorsFound) {

ShowFatalError(state, std::string{RoutineName} + "Errors discovered, program terminates.");

}

@jmarrec I think I will take the offer of just reverting the commit. That will allow the fix to go in without having to worry about this new pattern. I'll revert and test it out locally to make sure, then push it up and get this merged.

Myoldmopar

Not marking this approved or not yet, I need to think about it a bit more.

Myoldmopar · 2022-09-14T21:02:55Z

src/EnergyPlus/SurfaceGeometry.cc

                ErrorsFound = true;
+                continue;


I understand the sentiment here. If we could issue all 3 errors up front on the subsurface, it would be good to let the user know all 3 errors right away. Otherwise they will end up possibly running 3 iterations to solve all the issues. But I also understand the idea of wanting to avoid issues from subsequent checking. I could be swayed either way. Perhaps some extra description of the exact errors we are bypassing by each continue would help.

Myoldmopar · 2022-09-14T21:03:08Z

tst/EnergyPlus/unit/SurfaceGeometry.unit.cc

+        "   ..... Reference severe error count=1",
+        "   ..... Last severe error=FenestrationSurface:Detailed=\"SURFACE 8 - TRIANGULARWINDOW\", invalid Construction Name=\"WRONG CONSTRUCTION\".",
+    });
+    EXPECT_TRUE(compare_err_stream(error_string, true));


…face is deemed invalid, skip to the next" This reverts commit dcbda05.

…indow_Construction

Myoldmopar · 2022-09-15T19:19:11Z

Everything happy after reverting that. We can certainly discuss the idea of early exits, but for now this is a nice minimal change plus test. Thanks!

Myoldmopar · 2022-09-15T19:24:16Z

Not waiting on any CI to run this, merging it in.

jmarrec added 3 commits September 6, 2022 12:08

Minimal fix for #9331

59f5c65

More exit early in GetHTSubSurfaceData: as soon as a SubSurface is …

dcbda05

…deemed invalid, skip to the next

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Sep 6, 2022

jmarrec self-assigned this Sep 6, 2022

rraustad reviewed Sep 10, 2022

View reviewed changes

Myoldmopar reviewed Sep 14, 2022

View reviewed changes

jmarrec added this to the EnergyPlus 22.2 milestone Sep 15, 2022

Myoldmopar added 2 commits September 15, 2022 14:09

Revert "More exit early in GetHTSubSurfaceData: as soon as a SubSur…

d25c602

…face is deemed invalid, skip to the next" This reverts commit dcbda05.

Merge remote-tracking branch 'origin/develop' into 9331_Crash_Wrong_W…

68b829f

…indow_Construction

Myoldmopar merged commit 055c562 into develop Sep 15, 2022

Myoldmopar deleted the 9331_Crash_Wrong_Window_Construction branch September 15, 2022 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #9331 - Crash if window construction is zero due to bad construction name #9646

Fix #9331 - Crash if window construction is zero due to bad construction name #9646

jmarrec commented Sep 6, 2022

rraustad Sep 10, 2022

Myoldmopar Sep 14, 2022

jmarrec Sep 15, 2022 •

edited

Loading

jmarrec Sep 15, 2022 •

edited

Loading

Myoldmopar Sep 15, 2022

Myoldmopar left a comment

Myoldmopar Sep 14, 2022

Myoldmopar Sep 14, 2022

Myoldmopar commented Sep 15, 2022

Myoldmopar commented Sep 15, 2022

	GetDetShdSurfaceData(state, ErrorsFound, NumSurfs, TotDetachedFixed, TotDetachedBldg);

	GetRectDetShdSurfaceData(state, ErrorsFound, NumSurfs, TotRectDetachedFixed, TotRectDetachedBldg);

	GetHTSurfaceData(state,
	ErrorsFound,
	NumSurfs,
	TotHTSurfs,
	TotDetailedWalls,
	TotDetailedRoofs,
	TotDetailedFloors,
	state.dataSurfaceGeometry->BaseSurfCls,
	state.dataSurfaceGeometry->BaseSurfIDs,
	NeedToAddSurfaces);

	GetRectSurfaces(state,
	ErrorsFound,
	NumSurfs,
	TotRectExtWalls,
	TotRectIntWalls,
	TotRectIZWalls,
	TotRectUGWalls,
	TotRectRoofs,
	TotRectCeilings,
	TotRectIZCeilings,
	TotRectGCFloors,
	TotRectIntFloors,
	TotRectIZFloors,
	state.dataSurfaceGeometry->BaseSurfIDs,
	NeedToAddSurfaces);

	GetHTSubSurfaceData(state,
	ErrorsFound,
	NumSurfs,
	TotHTSubs,
	state.dataSurfaceGeometry->SubSurfCls,
	state.dataSurfaceGeometry->SubSurfIDs,
	AddedSubSurfaces,
	NeedToAddSubSurfaces);

	GetRectSubSurfaces(state,
	ErrorsFound,
	NumSurfs,
	TotRectWindows,
	TotRectDoors,
	TotRectGlazedDoors,
	TotRectIZWindows,
	TotRectIZDoors,
	TotRectIZGlazedDoors,
	state.dataSurfaceGeometry->SubSurfIDs,
	AddedSubSurfaces,
	NeedToAddSubSurfaces);

	GetAttShdSurfaceData(state, ErrorsFound, NumSurfs, TotShdSubs);

	GetSimpleShdSurfaceData(state, ErrorsFound, NumSurfs, TotOverhangs, TotOverhangsProjection, TotFins, TotFinsProjection);

	GetIntMassSurfaceData(state, ErrorsFound, NumSurfs);

	state.dataSurface->TotSurfaces = NumSurfs + AddedSubSurfaces + NeedToAddSurfaces + NeedToAddSubSurfaces;

	if (ErrorsFound) {
	ShowFatalError(state, std::string{RoutineName} + "Errors discovered, program terminates.");
	}

Fix #9331 - Crash if window construction is zero due to bad construction name #9646

Fix #9331 - Crash if window construction is zero due to bad construction name #9646

Conversation

jmarrec commented Sep 6, 2022

Pull request overview

Pull Request Author

Reviewer

rraustad Sep 10, 2022

Choose a reason for hiding this comment

Myoldmopar Sep 14, 2022

Choose a reason for hiding this comment

jmarrec Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

jmarrec Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Myoldmopar Sep 15, 2022

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar Sep 14, 2022

Choose a reason for hiding this comment

Myoldmopar Sep 14, 2022

Choose a reason for hiding this comment

Myoldmopar commented Sep 15, 2022

Myoldmopar commented Sep 15, 2022

jmarrec Sep 15, 2022 •

edited

Loading

jmarrec Sep 15, 2022 •

edited

Loading