Skip to content

Conversation

@meg-gupta
Copy link
Contributor

Add annotation in CodeGenNumberAllocator.cpp to suppress sal warning.
Restructure code in Encoder.cpp, the two function calls were throwing off
the analyzer.

@meg-gupta
Copy link
Contributor Author

meg-gupta commented Dec 22, 2016

@ThomsonTan, @leirocks can you please review ?
@ThomsonTan thanks for the pointer on __analysis_assume.

if (m_func->IsOOPJIT())
{
Js::ThrowMapEntry * throwMap = NativeCodeDataNewArrayNoFixup(m_func->GetNativeCodeDataAllocator(), Js::ThrowMapEntry, m_pragmaInstrToRecordMap->Count());
for (int32 i = 0; i < m_pragmaInstrToRecordMap->Count(); i++)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i < m_pragmaInstrToRecordMap->Count() [](start = 30, length = 37)

What's the problem in i < m_progmaInstrToRecordMap, type mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The calls to m_pragmaInstrToRecordMap->Count() for AllocatorArray and loop counter is not recognized as equivalent by the analyzer.

{
Js::ThrowMapEntry * throwMap = NativeCodeDataNewArrayNoFixup(m_func->GetNativeCodeDataAllocator(), Js::ThrowMapEntry, m_pragmaInstrToRecordMap->Count());
for (int32 i = 0; i < m_pragmaInstrToRecordMap->Count(); i++)
int allocSize = m_pragmaInstrToRecordMap->Count();
Copy link
Collaborator

Choose a reason for hiding this comment

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

int [](start = 12, length = 3)

Use int32 to declare allocSize to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
#else
Assert(sizeCat == sizeof(Js::JavascriptNumber));
__analysis_assume(sizeCat == sizeof(Js::JavascriptNumber));
Copy link
Collaborator

Choose a reason for hiding this comment

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

__analysis_assume(sizeCat == sizeof(Js::JavascriptNumber)); [](start = 12, length = 59)

Why only need this if RECYCLER_MEMORY_VERIFY is not defined? I don't see it is set when RECYCLER_MEMORY_VERIFY is defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

RECYCLER_MEMORY_VERIFY means it's checked build, which looks we don't run pre-fast with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no warning when RECYCLER_MEMORY_VERIFY is defined, pLocalNumber is allocated as :
pLocalNumber = (Js::JavascriptNumber*)alloca(sizeCat);
So at WriteProcessMemory(hProcess, (void*)number, pLocalNumber, sizeCat, NULL) there will be no warning from the analyzer, saying pLocalNumber should be >= sizeCat.

Also looks like when RECYCLER_MEMORY_VERIFY is defined sizeCat can be larger than sizeof(Js::JavascriptNumber).

@ThomsonTan
Copy link
Collaborator

:shipit:

1 similar comment
@leirocks
Copy link
Contributor

:shipit:

Add annotation in CodeGenNumberAllocator.cpp to suppress sal warning.
Restructure code in Encoder.cpp, the two function calls were throwing off
the analyzer.
@chakrabot chakrabot merged commit 5961803 into chakra-core:release/1.4 Dec 23, 2016
chakrabot pushed a commit that referenced this pull request Dec 23, 2016
Merge pull request #2279 from meg-gupta:fixsal

Add annotation in CodeGenNumberAllocator.cpp to suppress sal warning.
Restructure code in Encoder.cpp, the two function calls were throwing off
the analyzer.
chakrabot pushed a commit that referenced this pull request Dec 23, 2016
Merge pull request #2279 from meg-gupta:fixsal

Add annotation in CodeGenNumberAllocator.cpp to suppress sal warning.
Restructure code in Encoder.cpp, the two function calls were throwing off
the analyzer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants