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

Build failing on GCC arm due to _N defined as a macro #187

Closed
LnnrtS opened this issue Sep 9, 2022 · 13 comments · Fixed by #281
Closed

Build failing on GCC arm due to _N defined as a macro #187

LnnrtS opened this issue Sep 9, 2022 · 13 comments · Fixed by #281

Comments

@LnnrtS
Copy link

LnnrtS commented Sep 9, 2022

ctype.h on that platform defines

#define	_U	01
#define	_L	02
#define	_N	04
#define	_S	010
#define _P	020
#define _C	040
#define _X	0100
#define	_B	0200

which breaks mdspan in many places since some of those names are used as template parameters.

Names starting with an underscore and upper case letter are reserved by the standard to be used by an implementation so I think the proper way to handle this is rename the template parameters. I didn't find a quick way to automate the renaming otherwise I would have made a pull request.

@crtrott
Copy link
Member

crtrott commented Sep 10, 2022

Yeah this implementation is meant to go into the standard implementation hence we also use underscores, obviously we need to make it compatible with the ones we want to intregrate with. But seriously who injects single letter macros into the global namespace lol, almost as bad as MSVC definine MIN and MAX as macros ....

@crtrott
Copy link
Member

crtrott commented Sep 10, 2022

On the other side I don't think there is a reason we should put _ in front of tempalte parameters in the first place.

@mhoemmen
Copy link
Contributor

On the other side I don't think there is a reason we should put _ in front of tempalte parameters in the first place.

Right, I don't think it's our job necessarily to pre-mangle symbols. If a standard library implementation wants to adopt our reference mdspan implementation, they will do that mangling on their own.

This is a bit relevant to #185, though. We've been wanting to use a prefix _ as a way to mark macros as implementation details. On the other hand, "namespaced" macros like _MDSPAN_INLINE_FUNCTION probably won't collide with an existing standard library implementation's symbols.

@LnnrtS
Copy link
Author

LnnrtS commented Sep 12, 2022

Correct me if I'm wrong but I don't think the name of template parameters have to be mangled at all because they are no symbols and they don't leave the scope of the template definition. But - as we can see in this particular issue - they are affected by global macros. So consequently I would say they actually may not be mangled at all.

@mhoemmen
Copy link
Contributor

@LnnrtS wrote:

Correct me if I'm wrong but I don't think the name of template parameters have to be mangled at all because they are no symbols and they don't leave the scope of the template definition.

Just to clarify: The library in this repository does not need to mangle template parameters. Standard Library implementations of mdspan (which are coming) generally mangle symbols to avoid collisions with users' macros.

@LnnrtS
Copy link
Author

LnnrtS commented Sep 12, 2022

Ok I see. I wasn't taking user defined macros into account (I mean, why would anybody.. ;-))

@crtrott
Copy link
Member

crtrott commented Sep 24, 2022

We may wanna mangle but just use some larger names for the template parameters. That said I still think its insane to have in any library single letter macros, mangled or not.

@burnpanck
Copy link
Contributor

Did just run into this myself (also on arm bare-metal). I too, am aghast to hear of those single letter macros (which I wasn't aware of). Unfortunately, bare-metal development still is very much a C world, and you'd be surprised how much ugliness can be found in manufacturer's toolchains and SDKs. So here too, I'd would be grateful for any other choice of template parameter names, despite the fact that these macros being a crime against any sane programmer.

Unfortunately, these macros are actually used somewhere deep inside internal headers of the stdlib, so the best way I found to avoid them leaking into the rest of the code is to include some significant part of the stdlib (<ios> - something that I really don't need on embedded) early on in every file and then undefining the offending macros.

@crtrott
Copy link
Member

crtrott commented Mar 28, 2023

This PR I think will actually fix this incidentally: #212

@crtrott
Copy link
Member

crtrott commented Apr 4, 2023

I think this is now fixed can you guys try? @burnpanck @LnnrtS

@LnnrtS
Copy link
Author

LnnrtS commented Apr 10, 2023

Fixed, apart from _U in __compressed_pair

mhoemmen pushed a commit to mhoemmen/mdspan that referenced this issue Jul 26, 2023
…d-test-helpers

kokkos: refactor duplicated test helpers
@ehntoo
Copy link
Contributor

ehntoo commented Aug 11, 2023

A quick godbolt reproduction demonstrating the remaining breakage in __compressed_pair: https://godbolt.org/z/s6qzafWP4

I can put together a PR to fix the remaining issue, but I've got two questions:

  • Would renaming __compressed_pair's _T and _U template parameters to _T1 and _T2 be an acceptable fix?
  • Would an #ifdef _T1 ... #error "private macro already defined" -type check be welcomed? The current compile errors are truly mystifying, it could be nice to have a plainer set of errors if this were to occur again.

@mhoemmen
Copy link
Contributor

mhoemmen commented Aug 12, 2023

Would an #ifdef _T1 ... #error "private macro already defined" -type check be welcomed?

This check would require 3 times (number-of-template-parameters) lines of preprocessor logic for every class template and function template. There are so many class templates and function templates in this library. An important goal of the reference implementation of mdspan is to be legible.

If we find ourselves wanting to work around this issue, I would much prefer just picking different template parameter names.

Would renaming __compressed_pair's _T and _U template parameters to _T1 and _T2 be an acceptable fix?

I would find that acceptable.

ehntoo added a commit to ehntoo/mdspan that referenced this issue Aug 12, 2023
Newlib defines a macro _U in ctypes.h, resulting in a collision with
the existing template names for __compressed_pair. Use _T1 and _T2
instead of _T and _U to avoid this collision.

Fixes kokkos#187.
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 a pull request may close this issue.

5 participants