Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.2] reenable warnings for softfloat and wasm-jit, and fix a warning in wasm-jit #263

Merged
merged 7 commits into from
Oct 4, 2022

Conversation

linh2931
Copy link
Member

Resolve #256

Warnings were disabled for softfloat and wasm-jit. From #256:

The comment says these are third party libraries, and that is true, but they're also third party libraries we completely "own" in that they're part of the repo (wasm-jit) or forks that we maintain (softfloat). It's quite possible the warnings are benign, but these are important components of the software where maybe we'd like to know if something more critical ever showed up one day in a future compiler version or such.

After warnings were re-enabled, only following warnings were reported. They were not serious. Warning in wasm-jit is fixed in this PR. Warnings in softfloat will be fixed in a separate PR (as it is a submodule).

/home/lh/work/leap-main/libraries/wasm-jit/Source/WAST/NFA.cpp:517:54: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘NFA::CharSet’ {aka ‘struct DenseStaticIntSet<unsigned char, 256>’}; use    assignment or value-initialization instead [-Wclass-memaccess]
  517 |   memset(classCharSets,0,sizeof(CharSet) * numClasses);
      |                                                      ^
/home/lh/work/leap-main/libraries/softfloat/source/s_roundPackToF16.c:106:2: warning: label ‘packReturn’ defined   but not used [-Wunused-label]
  106 |  packReturn:
/home/lh/work/leap-main/libraries/softfloat/source/s_roundPackToF32.c:106:2: warning: label ‘packReturn’ defined   but not used [-Wunused-label]
  106 |  packReturn:
/home/lh/work/leap-main/libraries/softfloat/source/s_roundPackToF64.c:110:2: warning: label ‘packReturn’ defined   but not used [-Wunused-label]
  110 |  packReturn:

@@ -514,7 +514,7 @@ namespace NFA
std::set<StateIndex> terminalStates;

CharSet* classCharSets = (CharSet*)alloca(sizeof(CharSet) * numClasses);
memset(classCharSets,0,sizeof(CharSet) * numClasses);
memset((char*)classCharSets,0,sizeof(CharSet) * numClasses);
Copy link
Member

Choose a reason for hiding this comment

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

Does this just mask the warning? It's still memseting a non trivial type.

A CharSet is a DenseStaticIntSet<U8,256> and DenseStaticIntSet actually does do a memset on its contents. Since this isn't performance sensitive I wonder if just replacing this with a std::vector or such makes sense

Copy link
Member

Choose a reason for hiding this comment

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

dumpDFAGraphViz() is never called unless -D_DEBUG is passed, which it is not.

Copy link
Member

Choose a reason for hiding this comment

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

good call. just zap it all then

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean _DEBUG flag would never be used and I can go ahead removing all #ifdef _DEBUG and the functions which were only used inside #ifdef _DEBUG block like dumpDFAGraphViz?

Copy link
Member

Choose a reason for hiding this comment

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

Right, _DEBUG will never be set, so might as well just remove this code since it's not too invasive of change.

linh2931 and others added 4 commits October 2, 2022 09:37
dumpNFAGraphViz and dumpDFAGraphViz are only used in _DEBUG mode.
But _DEBUG will never be used. Remove them for simpler code and one fewer warning
#else
false,
#endif
false, // debug
Copy link
Member

Choose a reason for hiding this comment

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

wasm-jit has a different indentation style

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed. It was caused by a tab (wasm-jit uses tabs for indentation).

@linh2931 linh2931 merged commit 28f8ac1 into main Oct 4, 2022
@linh2931 linh2931 deleted the reenable_warning_softfloat_wasmjit branch October 4, 2022 17:26
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.

reconsider/investigate broad suppression of warnings on softfloat & wasm-jit
3 participants