[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze #46181

iarspider · 2024-10-01T13:34:21Z

PR description:

LLVM report: link

PR validation:

Bot tests

cmsbuild · 2024-10-01T13:34:48Z

cms-bot internal usage

cmsbuild · 2024-10-01T13:36:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46181/41978

cmsbuild · 2024-10-01T13:36:26Z

A new Pull Request was created by @iarspider for master.

It involves the following packages:

CalibTracker/SiPixelESProducers (alca)

@atpathak, @cmsbuild, @consuegs, @perrotta can you please review it and eventually sign? Thanks.
@dkotlins, @ferencek, @mmusich, @mroguljic, @rsreds, @tocheng, @tsusa, @tvami, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2024-10-01T13:46:08Z

CalibTracker/SiPixelESProducers/plugins/SiPixelFakeGenErrorDBObjectESSource.cc

@@ -35,9 +34,9 @@ std::unique_ptr<SiPixelGenErrorDBObject> SiPixelFakeGenErrorDBObjectESSource::pr
  //  open the GenError file(s)
  for (m = 0; m < obj->numOfTempl(); ++m) {
    edm::FileInPath file(GenErrorCalibrations_[m].c_str());
-    tempfile = (file.fullPath()).c_str();
+    std::string tempfile = file.fullPath();


@iarspider , isn't it better/faster to create std::string tempfile outside the for loop?

@Dr15Jones @makortel , is it intentional that FileInPath::fullPath returns a copy and not a const string&?

Not sure. If we really care about repeated (de)allocation of memory, maybe change fullPath to return reference?

is it intentional that FileInPath::fullPath returns a copy and not a const string&?

I'd guess "not intentional". Or, I can't think of any good reason why it returns a copy and not a const reference.

@makortel can you prepare a PR, or should I make one?

@iarspider It would be great if you could do it. On the same go I'd suggest to change relativePath() to return const string& as well, and remove the explicit definitions of copy constructor and assignment operator, and destructor.

smuzaffar · 2024-10-01T13:47:14Z

CalibTracker/SiPixelESProducers/plugins/SiPixelFakeGenErrorDBObjectESSource.cc


-    std::ifstream in_file(tempfile, std::ios::in);
+    std::ifstream in_file(tempfile.c_str(), std::ios::in);


will it work if we pass file.fullPath().c_str() directly here (without create tempfile string)?

tempfile is used also in else branch

frequency of hitting else is very low. So one can just use file.fullPath() there too ... right?

frequency of hitting else is very low. So one can just use file.fullPath() there too ... right?

I would think so.

mmusich · 2024-10-01T16:32:06Z

@iarspider can the title of the PR reflect the name of the changed class?

iarspider · 2024-10-11T20:57:11Z

please test with #46200

cmsbuild · 2024-10-11T20:59:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46181/42198

cmsbuild · 2024-10-11T20:59:39Z

Pull request #46181 was updated. @atpathak, @consuegs, @perrotta can you please check and sign again.

iarspider · 2024-10-11T21:23:54Z

please abort

iarspider · 2024-10-11T21:26:16Z

please test with #46200

cmsbuild · 2024-10-11T21:27:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46181/42199

cmsbuild · 2024-10-11T21:28:06Z

Pull request #46181 was updated. @atpathak, @consuegs, @francescobrivio, @perrotta can you please check and sign again.

cmsbuild · 2024-10-12T04:29:35Z

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85a4d0/42141/summary.html
COMMIT: 19c25e2
CMSSW: CMSSW_14_2_X_2024-10-11-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46181/42141/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

2022.1010012022.101001_RunEGamma2022C_10k/step1_dasquery.log
2022.101001DAS Error

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 40 differences found in the comparisons
DQMHistoTests: Total files compared: 44
DQMHistoTests: Total histograms compared: 3331066
DQMHistoTests: Total failures: 3445
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3327601
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
Checked 193 log files, 163 edm output root files, 44 DQM output files
TriggerResults: no differences found

iarspider · 2024-10-12T08:31:00Z

@cmsbuild ignore tests-rejected with external-failure
DAS query error is clearly not related to this PR, also happens in IBs

perrotta · 2024-10-12T12:42:50Z

+1

cmsbuild · 2024-10-12T12:43:11Z

This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @mandrenguyen, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2024-10-12T12:48:13Z

+1

cmsbuild added this to the CMSSW_14_2_X milestone Oct 1, 2024

cmsbuild added alca-pending pending-signatures tests-pending orp-pending code-checks-pending trk labels Oct 1, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 1, 2024

smuzaffar reviewed Oct 1, 2024

View reviewed changes

iarspider mentioned this pull request Oct 1, 2024

[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelTemplateDBObjectReader::analyze #46180

Closed

iarspider changed the title ~~[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer~~ [LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce Oct 1, 2024

iarspider mentioned this pull request Oct 2, 2024

edm::FileInPath: return reference to paths; cleanup code #46200

Merged

iarspider force-pushed the iarspider-patch-20241001-2 branch from 94fac8c to ce52df2 Compare October 11, 2024 20:56

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 11, 2024

cmsbuild added tests-started requires-external and removed tests-pending labels Oct 11, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 11, 2024

cmsbuild added tests-started requires-external and removed tests-pending labels Oct 11, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 11, 2024

cmsbuild added tests-rejected and removed tests-started labels Oct 12, 2024

cmsbuild added tests-approved tests-external-failure and removed tests-rejected labels Oct 12, 2024

cmsbuild added alca-approved fully-signed db-approved and removed alca-pending db-pending pending-signatures labels Oct 12, 2024

cmsbuild added orp-approved and removed orp-pending labels Oct 12, 2024

cmsbuild merged commit 32c7e17 into cms-sw:master Oct 12, 2024
10 of 11 checks passed

This was referenced Oct 15, 2024

[UBSAN][L1] Fix read out of bounds issue #46372

Closed

Update a few MC Gts in autoCond to the latest version available on 14/10/2024 #46379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze #46181

[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze #46181

iarspider commented Oct 1, 2024

cmsbuild commented Oct 1, 2024 •

edited

Loading

cmsbuild commented Oct 1, 2024

cmsbuild commented Oct 1, 2024

smuzaffar Oct 1, 2024

smuzaffar Oct 1, 2024 •

edited

Loading

iarspider Oct 1, 2024

makortel Oct 1, 2024

iarspider Oct 1, 2024

makortel Oct 2, 2024

smuzaffar Oct 1, 2024

iarspider Oct 1, 2024

smuzaffar Oct 1, 2024

iarspider Oct 1, 2024

mmusich commented Oct 1, 2024

iarspider commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

iarspider commented Oct 11, 2024

iarspider commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 12, 2024

iarspider commented Oct 12, 2024 •

edited

Loading

perrotta commented Oct 12, 2024

cmsbuild commented Oct 12, 2024

mandrenguyen commented Oct 12, 2024


		std::ifstream in_file(tempfile, std::ios::in);
		std::ifstream in_file(tempfile.c_str(), std::ios::in);

[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze #46181

[LLVM Analyzer] Avoid storing pointer to a temporary string's internal buffer in SiPixelFakeGenErrorDBObjectESSource::produce and SiPixelTemplateDBObjectReader::analyze #46181

Conversation

iarspider commented Oct 1, 2024

PR description:

PR validation:

cmsbuild commented Oct 1, 2024 • edited Loading

cmsbuild commented Oct 1, 2024

cmsbuild commented Oct 1, 2024

Choose a reason for hiding this comment

smuzaffar Oct 1, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich commented Oct 1, 2024

iarspider commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

iarspider commented Oct 11, 2024

iarspider commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 11, 2024

cmsbuild commented Oct 12, 2024

RelVals-INPUT

Comparison Summary

iarspider commented Oct 12, 2024 • edited Loading

perrotta commented Oct 12, 2024

cmsbuild commented Oct 12, 2024

mandrenguyen commented Oct 12, 2024

cmsbuild commented Oct 1, 2024 •

edited

Loading

smuzaffar Oct 1, 2024 •

edited

Loading

iarspider commented Oct 12, 2024 •

edited

Loading