Skip to content
This repository was archived by the owner on Mar 24, 2022. It is now read-only.

Add a config.h, containing GPOS_DEBUG and other flags that affect ABI.#32

Open
hlinnaka wants to merge 2 commits intovmware-archive:masterfrom
hlinnaka:config-h
Open

Add a config.h, containing GPOS_DEBUG and other flags that affect ABI.#32
hlinnaka wants to merge 2 commits intovmware-archive:masterfrom
hlinnaka:config-h

Conversation

@hlinnaka
Copy link
Contributor

Before this patch, consumers of libgpos had to know out-of-band which
flags were used to compile libgpos, because e.g. libgpos compiled with
GPOS_DEBUG would only work if the application using libgpos was also
compiled with GPOS_DEBUG. This is because many of the structs differ
depending on GPOS_DEBUG. Same for the architecture flags, like GPOS_i386.

The new config.h file is #included from a few central other header files,
to make sure it gets included in any application that uses other gpos
headers. We probably should include config.h from all other gpos header
files, to be sure, but this seems to be enough for ORCA and GPDB at least.

Before this patch, consumers of libgpos had to know out-of-band which
flags were used to compile libgpos, because e.g. libgpos compiled with
GPOS_DEBUG would only work if the application using libgpos was also
compiled with GPOS_DEBUG. This is because many of the structs differ
depending on GPOS_DEBUG. Same for the architecture flags, like GPOS_i386.

The new config.h file is #included from a few central other header files,
to make sure it gets included in any application that uses other gpos
headers. We probably should include config.h from *all* other gpos header
files, to be sure, but this seems to be enough for ORCA and GPDB at least.
@gpdbdreddbot
Copy link

Hello hlinnaka!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@hlinnaka
Copy link
Contributor Author

This is the same as PR #29. Let's see if it works this time. I have no idea what problem caused it to be reverted the first time 'round.

@d, please have a look.

@hlinnaka
Copy link
Contributor Author

@d
Copy link
Contributor

d commented Aug 15, 2016

-1

This looks like it will fail when the build type is debug. Can you try building both PR under CMake Build Type debug and run tests? My hunch is that orca won't even link

@hlinnaka
Copy link
Contributor Author

On 08/15/2016 05:15 PM, Jesse Zhang wrote:

-1

This looks like it will fail when the build type is debug. Can you
try building both PR under CMake Build Type debug and run tests? My
hunch is that orca won't even link

Works fine for me.

Can you please explain what the problem is that you're seeing? Are you
seeing an error? What is the error message?

  • Heikki

@d
Copy link
Contributor

d commented Aug 15, 2016

when you configure GPOS with:
cmake -DCMAKE_BUILD_TYPE=Debug /path/to/gpos

The generated / installed gpos/config.h will NOT contain the macro GPOS_DEBUG, this will cause ORCA test failure and (in the best case) linking error

@d
Copy link
Contributor

d commented Aug 15, 2016

The following past failed build might be of value to you, @hlinnaka https://gporca.ci.pivotalci.info/pipelines/gporca/jobs/orca_centos5_debug/builds/9

In Linux we were able to link, but the resulting binary was failing all tests (obviously)

@hlinnaka
Copy link
Contributor Author

hlinnaka commented Aug 15, 2016

On 08/15/2016 07:44 PM, Jesse Zhang wrote:

when you configure GPOS with:
cmake -DCMAKE_BUILD_TYPE=Debug /path/to/gpos

The generated / installed gpos/config.h will NOT contain the macro
GPOS_DEBUG, this will cause ORCA test failure and (in the best
case) linking error

A-ha, I didn't know about CMAKE_BUILD_TYPE! I had used just "cmake
-DGPOS_DEBUG=1". Thanks!

I'm baffled why that led to a linking error, though. I would expect ORCA
to just not be built with GPOS_DEBUG. In any case, I pushed a change
to this PR, to set GPOS_DEBUG, when CMAKE_BUILD_TYPE is Debug. Please
have a look.

  • Heikki

@d
Copy link
Contributor

d commented Aug 21, 2016

I'm not baffled how that could lead to linking error: it's C/C++ with #ifdef/#ifndef, which means your ABI is not constant, anything goes. In our case, the ABI varies depending on both the value of GPOS_DEBUG macro and the value of GPOS_DEBUG CMake variable.

@hlinnaka
Copy link
Contributor Author

On 21 August 2016 07:45:18 EEST, Jesse Zhang notifications@github.com wrote:

I'm not baffled how that could lead to linking error: it's C/C++ with
#ifdef/#ifndef, which means your ABI is not constant, anything
goes. In our case, the ABI varies depending on both the value of
GPOS_DEBUG macro and the value of GPOS_DEBUG CMake variable.

How does the cmake variable affect the ABI?

Do you still see a problem with this latest patch?

  • Heikki

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants