[ASAN] Fix runtime error by moving std::regex code in anonymous namespace #42077

smuzaffar · 2023-06-24T11:30:08Z

This fixes #42059 ASAN runtime issue. This looks like a GCC bug as reported in #40902 (comment) . Local tests shows that failing ASAN workflow runs after this change

smuzaffar · 2023-06-24T11:31:12Z

test parameters:
workflow = 20034.0

smuzaffar · 2023-06-24T11:31:18Z

please test

cmsbuild · 2023-06-24T11:37:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42077/36056

This PR adds an extra 16KB to repository

cmsbuild · 2023-06-24T11:37:27Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

L1Trigger/Phase2L1Taus (l1)

@epalencia, @aloeliger can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2023-06-24T11:44:17Z

please test for CMSSW_13_2_ASAN_X

VinInn · 2023-06-24T13:44:11Z

The use of regex for such a simple text replacement looks an overkill to me.

cmsbuild · 2023-06-24T14:22:30Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a1d9e/33371/summary.html
COMMIT: 07f077b
CMSSW: CMSSW_13_2_X_2023-06-23-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42077/33371/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 10 lines from the logs
Reco comparison results: 13 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3295604
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3295573
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 212 log files, 163 edm output root files, 49 DQM output files
TriggerResults: no differences found

aloeliger · 2023-06-24T14:28:16Z

+l1

cmsbuild · 2023-06-24T14:28:41Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

cmsbuild · 2023-06-24T15:08:31Z

-1

Failed Tests: RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a1d9e/33372/summary.html
COMMIT: 07f077b
CMSSW: CMSSW_13_2_ASAN_X_2023-06-23-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42077/33372/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

20034.020034.0_TTbar_14TeV+2026D86/step2_TTbar_14TeV+2026D86.log
24834.024834.0_TTbar_14TeV+2026D98/step2_TTbar_14TeV+2026D98.log
23234.023234.0_TTbar_14TeV+2026D94/step2_TTbar_14TeV+2026D94.log

Expand to see more relval errors ...

25034.999

smuzaffar · 2023-06-24T19:34:17Z

hold

looks like this did not fix the ASAN runtime errors

cmsbuild · 2023-06-24T19:34:37Z

Pull request has been put on hold by @smuzaffar
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

smuzaffar · 2023-06-24T19:36:04Z

The use of regex for such a simple text replacement looks an overkill to me.

I agree, may be we just use std::string replace here.

makortel · 2023-06-26T12:56:24Z

hold

looks like this did not fix the ASAN runtime errors

The failures are different from #42059

==3070823==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300b5678dc at pc 0x14db50c035a4 bp 0x7fff7a046290 sp 0x7fff7a046288
WRITE of size 2 at 0x60300b5678dc thread T0
    #0 0x14db50c035a3 in TauNNIdHW::compute(l1t::PFCandidate const&, std::vector<l1t::PFCandidate, std::allocator<l1t::PFCandidate> >&) (/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02790/el8_amd64_gcc11/cms/cmssw/CMSSW_13_2_ASAN_X_2023-06-23-2300/lib/el8_amd64_gcc11/libL1TriggerPhase2L1ParticleFlow.so+0x45c5a3)
...

that would be better to be followed up in a separate issue.

dan131riley · 2023-07-11T15:52:41Z

hold
looks like this did not fix the ASAN runtime errors

The failures are different from #42059 [...] that would be better to be followed up in a separate issue.

Agreed, the new failures are unrelated to the regex issue, this PR should not be kept on hold because of those new failures. The new ones come from

cmssw/L1Trigger/Phase2L1ParticleFlow/src/taus/TauNNIdHW.cc

Lines 91 to 104 in 546d4a7

    
           for (unsigned int i0 = 0; i0 < iParts.size(); i0++) { 
        
             if (i0 > fNParticles_) 
        
               break; 
        
             fPt_.get()[i0] = pt_t(iParts[i0].pt()); 
        
             fEta_.get()[i0] = etaphi_t(iSeed.eta() - iParts[i0].eta()); 
        
             etaphi_t lDPhi = etaphi_t(iSeed.phi()) - etaphi_t(iParts[i0].phi()); 
        
             etaphi_t lMPI = 3.1415; 
        
             if (lDPhi > lMPI) 
        
               lDPhi = lDPhi - lMPI; 
        
             if (lDPhi < -lMPI) 
        
               lDPhi = lDPhi + lMPI; 
        
             fPhi_.get()[i0] = lDPhi; 
        
             fId_.get()[i0] = id_t(iParts[i0].id()); 
        
           }

where the test at line 92 should '>=', not '>'. I don't know why we have not encountered this before, but I've confirmed that's the problem and will make a new PR.

dan131riley · 2023-07-11T16:07:26Z

Fix is in #42233

perrotta · 2023-07-11T16:20:52Z

please test for CMSSW_13_2_ASAN_X with #42233

perrotta · 2023-07-11T19:55:29Z

test parameters:

pull_request = fix off-by-one error in TauNNIdHW::compute #42233

perrotta · 2023-07-11T19:56:06Z

please test for CMSSW_13_2_ASAN_X

cmsbuild · 2023-07-11T23:58:37Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a1d9e/33647/summary.html
COMMIT: 07f077b
CMSSW: CMSSW_13_2_ASAN_X_2023-07-10-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42077/33647/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a1d9e/33647/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a1d9e/33647/git-merge-result

perrotta · 2023-07-12T05:31:32Z

@smuzaffar together with #42233 the runtime errors in ASAN are gone, as supposed by @dan131riley : I think this can be unheld and merged, then

smuzaffar · 2023-07-12T10:36:56Z

unhold

cmsbuild · 2023-07-12T10:37:20Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2023-07-12T10:39:23Z

+1

ASAN] Fix runtime error by moving std::regex code in anonymous namespace

07f077b

smuzaffar mentioned this pull request Jun 24, 2023

[ASAN] global-buffer-overflow in L1HPSPFTauBuilder constructor #42059

Closed

cmsbuild added this to the CMSSW_13_2_X milestone Jun 24, 2023

cmsbuild added l1-pending pending-signatures tests-pending orp-pending code-checks-pending labels Jun 24, 2023

cmsbuild added tests-started and removed tests-pending labels Jun 24, 2023

aloeliger mentioned this pull request Jun 24, 2023

[ASAN] global-buffer-overflow in L1HPSPFTauBuilder constructor cms-l1t-offline/cmssw#1141

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 24, 2023

cmsbuild added tests-approved and removed tests-started labels Jun 24, 2023

cmsbuild added l1-approved fully-signed and removed l1-pending pending-signatures labels Jun 24, 2023

cmsbuild added pending-signatures hold and removed fully-signed labels Jun 24, 2023

dan131riley mentioned this pull request Jul 11, 2023

fix off-by-one error in TauNNIdHW::compute #42233

Merged

cmsbuild added the requires-external label Jul 11, 2023

cmsbuild added fully-signed and removed pending-signatures hold labels Jul 12, 2023

cmsbuild added orp-approved and removed orp-pending labels Jul 12, 2023

cmsbuild merged commit 5aae029 into cms-sw:master Jul 12, 2023

[ASAN] Fix runtime error by moving std::regex code in anonymous namespace #42077

[ASAN] Fix runtime error by moving std::regex code in anonymous namespace #42077

Uh oh!

Conversation

smuzaffar commented Jun 24, 2023

Uh oh!

smuzaffar commented Jun 24, 2023

Uh oh!

smuzaffar commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

Uh oh!

smuzaffar commented Jun 24, 2023

Uh oh!

VinInn commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

Comparison Summary

Uh oh!

aloeliger commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

RelVals

Uh oh!

smuzaffar commented Jun 24, 2023

Uh oh!

cmsbuild commented Jun 24, 2023

Uh oh!

smuzaffar commented Jun 24, 2023

Uh oh!

makortel commented Jun 26, 2023

Uh oh!

dan131riley commented Jul 11, 2023

Uh oh!

dan131riley commented Jul 11, 2023

Uh oh!

perrotta commented Jul 11, 2023

Uh oh!

perrotta commented Jul 11, 2023

Uh oh!

perrotta commented Jul 11, 2023

Uh oh!

cmsbuild commented Jul 11, 2023

Uh oh!

perrotta commented Jul 12, 2023

Uh oh!

smuzaffar commented Jul 12, 2023

Uh oh!

cmsbuild commented Jul 12, 2023

Uh oh!

perrotta commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants