Segfault when trying to use an empty line

[Former user]

BDSIM will segfault when trying to use an empty line. See attached file with:

bdsim --file=empty_line_segfault.gmad

[Former user]

This appears to be a general problem when trying to run BDSIM with an empty beamline.

Same problem if you don't have:

use, period=lat;

in your code.

[Jochem Snuverink]

Thanks for the report. Is there a use case for an empty line? Can we just return with a warning message early?

I tried fixing this quickly but there are several places that assume a non-empty beamline, and I am wondering if this is the right approach.

[Jochem Snuverink]

I have added some protection in cab4912. There should be no segfault anymore. Still the question remains if rather a warning message should be given.

[Laurie Nevay]

I was under the impression that the parser would throw an error for an empty beam line, but it perhaps makes more sense this check is done in BDSIM rather than in the parser.

I think, for now, we require that there be a beam line and that we have at least 1 element in it. The alternative would be for using BDSIM to simply create a Geant4 model of externally placed geometry.

I've added a check for this up front in the detector construction, but the parser fast list does not behave as expected. I created the empty method using list::empty but the beam line always contains at least one element which is the 'line' type itself. As the beam line is always expanded to a flat sequence, this is perhaps a bit unintuitive.

If we maintain a sequence in the parser that contains lines that are other sequences, fair enough, but I think the flat representation (the one BDSIM uses to build the model) should have the line elements removed.

I've created a test that passes when the error is thrown for an empty beam line. It currently fails because the parser behaviour described.

[Laurie Nevay]

Thanks for fixing the segfault by the way. Aside from principles of what the code does, we should never segfault.

[Jochem Snuverink]

Some remarks:

• If you leave out use, period=lat the list is empty and the early exiting works.

• I think we could leave out the 'line' itself from the expansion, but attaching a sampler to a line wouldn't work anymore, and there might be more consequences, I am not sure.

• Alternatively, instead of probing the list directly, one could write a similar method as BDSDetectorConstruction::UnsuitableFirstElement() that checks if there is a non-line element.

• I think such methods should by the way rather be in BDSParser, so that there is a single entry class (perhaps together with BDSComponentFactory) that communicates with the parser objects (the original design).

• I just noticed that circular with an empty line will still segfault. --> EDIT: fixed in ba846b0

[Former user]

To provide a use case for having no line: I originally discovered this bug when trying to view a global placement of an external geometry with no line. However it is not too difficult to get around this missing feature, as you can simply add an arbitrary external geometry to a line with use off the element component class.

[Jochem Snuverink]

One could of course also do the check after/in the BuildBeamline call. Then the flatBeamline is available as made by the ComponentFactory.

[Laurie Nevay]

Thanks for that. A few thoughts.

• Please can tests be added for these extra cases. These can be one to test an expected failure with the 'simple_fail' test macro.
• We could indeed have a different method, but I feel it's better to emulate the standard library as much as possible as it's familiar and easier to use / develop.
• We're exposing internal workings of the parser to the large code. Ideally we should have a way to access the fully flattened / expanded beam line with all the samplers marked as required.
• I think this method is more a property of the fast list itself than something that belongs in BDSParser as we could have multiple beam lines.

[Laurie Nevay]

Just to clarify: BuildBeamline is now a method that is used multiple times to construct different beam lines, not just the main beam line. BuildBeamlines makes other extra ones after the main one has been constructed.

We could check in BuildBeamlines after it's made this main beamline, but I think we can check before on the fast list rather than use feature of the other container BDSBeamline. We'd have to be careful about cleaning up anything we'd already constructed at this point. I think it's easier to fix or implement a different method in the fastlist container.

[Jochem Snuverink]

fastlist is basically a list with a map, it is pure c++. It knows nothing about a beamline or elements. Therefore, the check should not be there.

In the test (agreed good to add more of them) there is:

BDSDetectorConstruction::BuildBeamline> main beam line BDSBeamline with 0 elements

The check would be very simple with minimum code after that.

We'd have to be careful about cleaning up anything we'd already constructed at this point.

If we are going to exit this doesn't matter so much.

We're exposing internal workings of the parser to the large code.

Yes, but this is a pragmatic decision, and we should avoid the need for it as much as possible (IMHO).

[Jochem Snuverink]

One other argument for checking on BDSBeamline rather than in the parser is that the parser won't know what will turn out to be a non-empty beamline. For example, there might be GMAD::Elements with zero lengths that are not made into a BDSComponent.

[Jochem Snuverink]

more tests were added in 74d64b1:

Current result:

$ ctest -R beam_line 
Test project /home/scratch/bdsim/build
    Start 168: no_beam_line
1/4 Test #168: no_beam_line .....................   Passed    0.23 sec
    Start 169: empty_beam_line
2/4 Test #169: empty_beam_line ..................***Failed    0.49 sec
    Start 170: empty_circular_beam_line
3/4 Test #170: empty_circular_beam_line .........***Failed    0.49 sec
    Start 171: zero_beam_line
4/4 Test #171: zero_beam_line ...................***Failed    0.48 sec

25% tests passed, 3 tests failed out of 4

Total Test time (real) =   4.71 sec

The following tests FAILED:
    169 - empty_beam_line (Failed)
    170 - empty_circular_beam_line (Failed)
    171 - zero_beam_line (Failed)
Errors while running CTest

[Jochem Snuverink]

The following change passes all 4 tests:

-  const auto& mainBeamLineFL = BDSParser::Instance()->GetBeamline();
- 
-  if (mainBeamLineFL.empty())
-    {
-      G4cerr << __METHOD_NAME__ << "BDSIM requires the sequence defined with the use command "
-            << "to have at least one element" << G4endl;
-      exit(1);
-    }
   BDSBeamlineSet mainBeamline = BuildBeamline(BDSParser::Instance()->GetBeamline(),
                                              "main beam line",
                                              initialTransform,
                                              circular);

+  if (mainBeamline.massWorld->empty())
+    {
+      G4cerr << __METHOD_NAME__ << "BDSIM requires the sequence defined with the use command "
+            << "to have at least one element" << G4endl;
+      exit(1);
+    }
+

@nevay : shall I push this?

[Laurie Nevay]

@jsnuverink Sounds good. Please go ahead. Thanks for fixing.

[Jochem Snuverink]

fix issue #212 : exit when main BDSBeamline is empty

→ <<cset 3c7d69b804dd>>